diff --git a/frontend/src/__tests__/ladder-timeline-api.test.ts b/frontend/src/__tests__/ladder-timeline-api.test.ts index bc645984b..70f37d7b0 100644 --- a/frontend/src/__tests__/ladder-timeline-api.test.ts +++ b/frontend/src/__tests__/ladder-timeline-api.test.ts @@ -1,6 +1,6 @@ jest.mock('../api/client', () => ({ apiRequest: jest.fn() })); -import { getLadderEvents, type LadderEvent } from '../api/ladder'; +import { getLadderEvents, getLadderRuns, type LadderEvent, type LadderRun } from '../api/ladder'; import { apiRequest } from '../api/client'; beforeEach(() => jest.resetAllMocks()); @@ -31,3 +31,13 @@ test('OD cross-check compares the displayed od_usd_hr values and tolerates a pre jest.mocked(apiRequest).mockResolvedValueOnce({ total_count: 1, total_usd_hr: '5.000000', events: [legacy] }); expect(await getLadderEvents('account', 'aws')).toHaveLength(1); }); + +// #1792: legacy rows say "cancelled", new ones "canceled"; the loader returns one spelling. +test('loaders normalize both stored spellings of canceled', async () => { + const event: LadderEvent = { id: 'old', config_id: 'config', run_id: 'run', layer_type: 'compute-sp', term: '1yr', payment_option: 'no-upfront', status: 'cancelled', run_status: 'cancelled', revision: 0, amount_usd_hr: '1.000000', scheduled_date: '2030-01-01T00:00:00Z', created_at: '2026-01-01T00:00:00Z' }; + jest.mocked(apiRequest).mockResolvedValueOnce({ total_count: 3, total_usd_hr: '3.000000', events: [event, { ...event, id: 'new', status: 'canceled', run_status: 'canceled' }, { ...event, id: 'live', status: 'scheduled', run_status: 'planned' }] }); + expect((await getLadderEvents('account', 'aws')).map(e => [e.status, e.run_status])).toEqual([['canceled', 'canceled'], ['canceled', 'canceled'], ['scheduled', 'planned']]); + const run = { id: 'r1', config_id: 'config', status: 'cancelled', started_at: '2026-01-01T00:00:00Z', created_at: '2026-01-01T00:00:00Z', baseline_usd_hr: null, target_usd_hr: null, existing_usd_hr: null, actions: [] } as unknown as LadderRun; + jest.mocked(apiRequest).mockResolvedValueOnce({ total_count: 2, runs: [run, { ...run, id: 'r2', status: 'executing' }] }); + expect((await getLadderRuns('account', 'aws')).map(r => r.status)).toEqual(['canceled', 'executing']); +}); diff --git a/frontend/src/__tests__/ladder-timeline-filters.test.ts b/frontend/src/__tests__/ladder-timeline-filters.test.ts index a1af45a2a..85773fb43 100644 --- a/frontend/src/__tests__/ladder-timeline-filters.test.ts +++ b/frontend/src/__tests__/ladder-timeline-filters.test.ts @@ -39,8 +39,8 @@ test('eligibility covers status, execution, deadline, and unavailable revision A test('ladder labels cover every known enum and show unknown codes raw', () => { expect(['ec2-instance-sp', 'compute-sp', 'convertible-ri', 'azure-reservation', 'azure-savings-plan'].map(v => ladderLabel('layer_type', v))) .toEqual(['EC2 Instance Savings Plan', 'Compute Savings Plan', 'Convertible RI', 'Azure Reservation', 'Azure Savings Plan']); - expect(['scheduled', 'fired', 'completed', 'cancelled', 'failed'].map(v => ladderLabel('status', v))) - .toEqual(['Scheduled', 'Fired', 'Completed', 'Cancelled', 'Failed']); + expect(['scheduled', 'fired', 'completed', 'canceled', 'failed'].map(v => ladderLabel('status', v))) + .toEqual(['Scheduled', 'Fired', 'Completed', 'Canceled', 'Failed']); expect(['1yr', '3yr'].map(v => ladderLabel('term', v))).toEqual(['1 Year', '3 Years']); expect(['no-upfront', 'partial-upfront', 'all-upfront'].map(v => ladderLabel('payment_option', v))).toEqual(['No Upfront', 'Partial Upfront', 'All Upfront']); expect(ladderLabel('layer_type', 'new-thing')).toBe('new-thing'); @@ -66,7 +66,7 @@ test('sub-0.0001 amounts are not shown as $0.0000 and edit reasons use readable expect(formatLadderAmount('0.000040')).toBe('<$0.0001/hr'); expect(formatLadderAmount('0.000100')).toBe('$0.0001/hr'); expect(ladderEditReason({ ...event, run_status: 'awaiting_approval' })).toBe('Run is awaiting approval.'); - expect(ladderEditReason({ ...event, status: 'cancelled' })).toBe('Tranche is cancelled.'); + expect(ladderEditReason({ ...event, status: 'canceled' })).toBe('Tranche is canceled.'); }); // FIXTURE: hand-written tranches. @@ -81,15 +81,15 @@ test('ladderCumulative carries each layer forward on shared times and keeps zero expect([...result.rowTotals]).toEqual([['a', 2500000n], ['b', 3500000n], ['c', 3500000n]]); }); -// FIXTURE: hand-written statuses; the gate repro was scheduled 1.0 + cancelled 5.0 + failed 7.0 showing $13.00/hr instead of $1.00/hr. +// FIXTURE: hand-written statuses; the gate repro was scheduled 1.0 + canceled 5.0 + failed 7.0 showing $13.00/hr instead of $1.00/hr. test('ladderCumulative counts only scheduled, fired and completed tranches (#698 gate)', () => { const at = (id: string, status: string, amount: string, day: number) => ({ ...event, id, status, amount_usd_hr: amount, scheduled_date: `2030-01-0${day}T00:00:00Z` }); - const rows = [at('s', 'scheduled', '1.000000', 1), at('c', 'cancelled', '5.000000', 2), at('f', 'failed', '7.000000', 3), at('x', 'weird', '9.000000', 4), at('fi', 'fired', '2.000000', 5), at('d', 'completed', '3.000000', 6)]; + const rows = [at('s', 'scheduled', '1.000000', 1), at('c', 'canceled', '5.000000', 2), at('f', 'failed', '7.000000', 3), at('x', 'weird', '9.000000', 4), at('fi', 'fired', '2.000000', 5), at('d', 'completed', '3.000000', 6)]; const result = ladderCumulative(rows, 0n); expect(result.totals).toEqual([1000000n, 3000000n, 6000000n]); expect([...result.rowTotals]).toEqual([['s', 1000000n], ['c', null], ['f', null], ['x', null], ['fi', 3000000n], ['d', 6000000n]]); expect(rows.map(ladderNotCountedNote)).toEqual([undefined, 'Not counted', 'Not counted', 'Not counted (unknown status)', undefined, undefined]); - expect(ladderCumulative([at('c', 'cancelled', '5.000000', 1)], 0n).times).toEqual([]); + expect(ladderCumulative([at('c', 'canceled', '5.000000', 1)], 0n).times).toEqual([]); }); test('typed UTC filter dates parse strictly and whole days cover both edges (issue #753)', () => { diff --git a/frontend/src/__tests__/ladder-timeline.test.ts b/frontend/src/__tests__/ladder-timeline.test.ts index 190eeb4ec..de0ac5023 100644 --- a/frontend/src/__tests__/ladder-timeline.test.ts +++ b/frontend/src/__tests__/ladder-timeline.test.ts @@ -315,7 +315,7 @@ test('a missing design token fails loudly instead of painting a default color (# // FIXTURE: hand-written tranches reproducing the gate finding (status filter cleared so all statuses are visible). test('cancelled, failed and unknown tranches stay visible but are not counted in chart, summary or table (#698 gate)', async () => { const row = (id: string, status: string, amount: string, day: number) => ({ ...event, id, status, amount_usd_hr: amount, scheduled_date: `2030-01-0${day}T00:00:00Z` }); - jest.mocked(getLadderEvents).mockResolvedValue([row('s', 'scheduled', '1.000000', 1), row('c', 'cancelled', '5.000000', 2), row('f', 'failed', '7.000000', 3), row('x', 'weird', '9.000000', 4)]); + jest.mocked(getLadderEvents).mockResolvedValue([row('s', 'scheduled', '1.000000', 1), row('c', 'canceled', '5.000000', 2), row('f', 'failed', '7.000000', 3), row('x', 'weird', '9.000000', 4)]); await initLadderTimeline(document.getElementById('timeline')!, [], false); const status = document.querySelector('[data-filter="status"]')!; status.value = ''; status.dispatchEvent(new Event('change')); @@ -336,7 +336,7 @@ test('layers are told apart by point shape as well as color, and a selection wit await initLadderTimeline(document.getElementById('timeline')!, [], false); const config = jest.mocked(Chart).mock.calls[0]![1] as { data: { datasets: { pointStyle: string }[] } }; expect(new Set(config.data.datasets.map(d => d.pointStyle)).size).toBe(2); - jest.mocked(getLadderEvents).mockResolvedValue([{ ...event, status: 'cancelled' }]); + jest.mocked(getLadderEvents).mockResolvedValue([{ ...event, status: 'canceled' }]); document.body.innerHTML = '
'; jest.mocked(Chart).mockClear(); await initLadderTimeline(document.getElementById('timeline')!, [], false); diff --git a/frontend/src/api/ladder.ts b/frontend/src/api/ladder.ts index cd184b6d7..eef6be0a4 100644 --- a/frontend/src/api/ladder.ts +++ b/frontend/src/api/ladder.ts @@ -174,12 +174,18 @@ async function completeTimeline( return items; } -export function getLadderEvents(accountID: string, provider: string): Promise { - return completeTimeline('tranches', accountID, provider); +// Rows written before cloud-commitments-go#554 keep the legacy "cancelled"; +// one spelling here keeps labels, filters and counts on a single value (#1792). +const canceledSpelling = (status: string): string => (status === 'cancelled' ? 'canceled' : status); + +export async function getLadderEvents(accountID: string, provider: string): Promise { + const events = await completeTimeline('tranches', accountID, provider); + return events.map(event => ({ ...event, status: canceledSpelling(event.status), run_status: canceledSpelling(event.run_status) })); } -export function getLadderRuns(accountID: string, provider: string): Promise { - return completeTimeline('runs', accountID, provider); +export async function getLadderRuns(accountID: string, provider: string): Promise { + const runs = await completeTimeline('runs', accountID, provider); + return runs.map(run => ({ ...run, status: canceledSpelling(run.status) })); } export interface LadderAmendment { diff --git a/frontend/src/ladder-timeline-filters.ts b/frontend/src/ladder-timeline-filters.ts index f73694dbe..94608d640 100644 --- a/frontend/src/ladder-timeline-filters.ts +++ b/frontend/src/ladder-timeline-filters.ts @@ -62,12 +62,12 @@ const layerLabels: Record = { 'azure-reservation': 'Azure Reservation', 'azure-savings-plan': 'Azure Savings Plan', }; const statusLabels: Record = { - scheduled: 'Scheduled', fired: 'Fired', completed: 'Completed', cancelled: 'Cancelled', failed: 'Failed', + scheduled: 'Scheduled', fired: 'Fired', completed: 'Completed', canceled: 'Canceled', failed: 'Failed', }; const runStatusLabels: Record = { planned: 'planned', awaiting_approval: 'awaiting approval', approved: 'approved', executing: 'executing', - completed: 'completed', failed: 'failed', cancelled: 'cancelled', expired: 'expired', + completed: 'completed', failed: 'failed', canceled: 'canceled', expired: 'expired', }; /** Label for a ladder enum code. An unknown code is shown raw so it stays visible. */ @@ -130,9 +130,9 @@ export function formatLadderUtcInput(value: string): string { return `${date} ${time.endsWith(':00') ? time.slice(0, 5) : time}`; } -/** Statuses that count toward cumulative commitment; cancelled and failed tranches were never bought. */ +/** Statuses that count toward cumulative commitment; canceled and failed tranches were never bought. */ export const ladderCountedStatuses: readonly string[] = ['scheduled', 'fired', 'completed']; -export const LADDER_COUNTED_NOTE = 'Counts scheduled, fired and completed tranches; cancelled, failed and unrecognised statuses are excluded.'; +export const LADDER_COUNTED_NOTE = 'Counts scheduled, fired and completed tranches; canceled, failed and unrecognised statuses are excluded.'; /** Note for a row that is excluded from the cumulative, or undefined when it is counted. */ export function ladderNotCountedNote(event: LadderEvent): string | undefined { diff --git a/go.mod b/go.mod index e8945c471..ee1314449 100644 --- a/go.mod +++ b/go.mod @@ -88,10 +88,10 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys v1.5.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.5.0 - github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261106191535-c83aef8b18a9 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261106191535-c83aef8b18a9 - github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261106191535-c83aef8b18a9 - github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261106191535-c83aef8b18a9 + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261107014806-0b677e19ae94 + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261107014806-0b677e19ae94 + github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261107014806-0b677e19ae94 + github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261107014806-0b677e19ae94 github.com/aws/aws-lambda-go v1.55.1 github.com/aws/aws-sdk-go-v2/service/kms v1.62.1 github.com/aws/aws-sdk-go-v2/service/lambda v1.113.4 diff --git a/go.sum b/go.sum index 60277f83d..17d1e6340 100644 --- a/go.sum +++ b/go.sum @@ -90,14 +90,14 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0 github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.57.0/go.mod h1:dzcEjy1WJ0Q4u9twNR3LcLhNoYMRCrMCMafpxa0TjPQ= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.57.0 h1:RoO5+d7uCmDqovLrHCr2/BuViUXvdcrNxyNM1pN9dDQ= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.57.0/go.mod h1:YqwkQPrWSC7+byyc1VlKbWLBF5JsW5IoL6xUkemYSXk= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261106191535-c83aef8b18a9 h1:omDAY4B4wshoedBYE368TW05d0PR2HTnebPgu8WqIFw= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261106191535-c83aef8b18a9/go.mod h1:l9V+ecJCfAigYr6XiK9kpIUemF9bnXOKHJN1s4TPsM0= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261106191535-c83aef8b18a9 h1:z4LNOkM5qTNq7dbzfdh0VdsSW4BTxRuVaXjOZfRADe0= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261106191535-c83aef8b18a9/go.mod h1:uVh1sdSIwKbHCWZ/TA66m2cR9Odb/36sxDHNKLyuMKY= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261106191535-c83aef8b18a9 h1:OISjPdyg7xcy8kCJelHJle4YnTMQycAWzglVi6L15uw= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261106191535-c83aef8b18a9/go.mod h1:X+73EdStvYV6pqEVZZFyMfLG1frORlO9B1IsuK0LfOs= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261106191535-c83aef8b18a9 h1:7Vz6QVVlwe0GG7TT2+4tQp9HBBCZ0YkfEDwFZOF+Ir8= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261106191535-c83aef8b18a9/go.mod h1:kfbQ5CsDr1958iSuZq6edcuB0es62XRCXGn/hFFYBW0= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261107014806-0b677e19ae94 h1:4AYCeaAqeNBKKLs6Q/FeIRWXCWFUe59/RiBrY9O39Xw= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261107014806-0b677e19ae94/go.mod h1:l9V+ecJCfAigYr6XiK9kpIUemF9bnXOKHJN1s4TPsM0= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261107014806-0b677e19ae94 h1:J7e8eVErWWG+yP80X2GAv5/dTTXQemejLXC1qYsvBWk= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261107014806-0b677e19ae94/go.mod h1:uVh1sdSIwKbHCWZ/TA66m2cR9Odb/36sxDHNKLyuMKY= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261107014806-0b677e19ae94 h1:55Yb/A/GF5yDCH+AxUnHbVZFMFNKMVjxipkgxgBLwss= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261107014806-0b677e19ae94/go.mod h1:h6EewdM42n97XHBC3/aMxZFAMUOoEla4Y18TDYtfC6w= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261107014806-0b677e19ae94 h1:7k9LzEBsSzF2HQFlUDDm6AMj/hs4VKLbL6MoC3Ghkq4= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261107014806-0b677e19ae94/go.mod h1:kfbQ5CsDr1958iSuZq6edcuB0es62XRCXGn/hFFYBW0= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU= github.com/agext/levenshtein v1.2.1 h1:QmvMAjj2aEICytGiWzmxoE0x2KZvE0fvmqMOfy2tjT8= diff --git a/internal/api/handler_purchases_retry_ladder_canceled_test.go b/internal/api/handler_purchases_retry_ladder_canceled_test.go new file mode 100644 index 000000000..888278637 --- /dev/null +++ b/internal/api/handler_purchases_retry_ladder_canceled_test.go @@ -0,0 +1,28 @@ +package api + +import ( + "context" + "errors" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/ladder" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// #1792: LockLadderTrancheForRetryTx returns the stored tranche status raw +// (store_postgres_ladder_reconcile.go). Rows written before +// cloud-commitments-go#554 keep the double-l spelling, new ones write "canceled"; Retry +// must refuse a canceled tranche under either spelling. +func TestGuardLadderTrancheForRetry_RefusesBothCanceledSpellings(t *testing.T) { + for _, stored := range []string{"canceled", "cancelled"} { //nolint:misspell // legacy stored ladder status value + t.Run(stored, func(t *testing.T) { + store := new(MockConfigStore) + store.On("LockLadderTrancheForRetryTx", mock.Anything, mock.Anything, "tranche-key"). + Return(ladder.TrancheStatus(stored), true, nil) + h := &Handler{config: store} + err := h.guardLadderTrancheForRetry(context.Background(), nil, "tranche-key") + require.True(t, errors.Is(err, errRetryLadderTrancheSettled), "a %q tranche must refuse Retry, got %v", stored, err) + }) + } +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index e3d7aa68b..5cccd3f19 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -9349,10 +9349,12 @@ components: enum: [all-upfront, partial-upfront, no-upfront] status: type: string - enum: [scheduled, fired, completed, cancelled, failed] + description: New rows write "canceled"; rows written before cloud-commitments-go#554 keep "cancelled". + enum: [scheduled, fired, completed, canceled, cancelled, failed] run_status: type: string - enum: [planned, awaiting_approval, approved, executing, completed, failed, cancelled, expired] + description: New rows write "canceled"; rows written before cloud-commitments-go#554 keep "cancelled". + enum: [planned, awaiting_approval, approved, executing, completed, failed, canceled, cancelled, expired] execution_id: type: string description: Omitted until the tranche has an execution. @@ -9405,7 +9407,8 @@ components: format: uuid status: type: string - enum: [planned, awaiting_approval, approved, executing, completed, failed, cancelled, expired] + description: New rows write "canceled"; rows written before cloud-commitments-go#554 keep "cancelled". + enum: [planned, awaiting_approval, approved, executing, completed, failed, canceled, cancelled, expired] started_at: type: string format: date-time diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index a2f03fc70..bb465562f 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -881,7 +881,7 @@ type StoreInterface interface { // current status), so callers can distinguish a race from a hard error. // Statuses are typed ladder.RunStatus so callers cannot pass an arbitrary // string that would never match a stored status. A transition to - // RunStatusCancelled is refused (ErrLadderRunCancelViaTransition). + // RunStatusCanceled is refused (ErrLadderRunCancelViaTransition). // // CancelLadderRun is the only way to cancel a ladder run: a CAS UPDATE // from one of fromStatuses to cancelled that stamps cancelled_by with the diff --git a/internal/config/recommendations_page.go b/internal/config/recommendations_page.go index d213aad71..2174a28b2 100644 --- a/internal/config/recommendations_page.go +++ b/internal/config/recommendations_page.go @@ -99,6 +99,16 @@ func SuppressionTupleKey(accountID, provider, service, region, resourceType, eng return accountID + sep + provider + sep + service + sep + region + sep + resourceType + sep + engine } +// SuppressionEngineSpellings returns every engine spelling a stored rec can +// carry that CanonicalSuppressionEngine maps to canonical. +func SuppressionEngineSpellings(provider, service, canonical string) []string { + out := []string{canonical} + if CanonicalSuppressionEngine(provider, service, "") == canonical && canonical != "" { + out = append(out, "") + } + return out +} + // SuppressionServiceSpellings returns every service spelling a stored rec // can carry that CanonicalSuppressionService maps to canonical for provider: // canonical itself (when it maps to itself) and its legacy and dash-free diff --git a/internal/config/store_postgres_ladder.go b/internal/config/store_postgres_ladder.go index 90ffed86d..15bd63215 100644 --- a/internal/config/store_postgres_ladder.go +++ b/internal/config/store_postgres_ladder.go @@ -352,7 +352,7 @@ func cancelOverdueLadderTranchesTx(ctx context.Context, tx pgx.Tx, configID *str AND status = $3 AND execution_id IS NULL AND scheduled_date <= $4 - `, string(ladder.TrancheStatusCancelled), *configID, string(ladder.TrancheStatusScheduled), ladderGraceCutoff(asOf)) + `, string(ladder.TrancheStatusCanceled), *configID, string(ladder.TrancheStatusScheduled), ladderGraceCutoff(asOf)) if err != nil { return fmt.Errorf("failed to cancel overdue ladder_tranches config_id=%s: %w", *configID, err) } @@ -438,11 +438,11 @@ func (s *PostgresStore) LatestLadderRunStartedAt(ctx context.Context, configID s // one of fromStatuses to toStatus via a CAS UPDATE. Returns the updated row // on success, or (nil, nil) when zero rows are affected (race lost or // unexpected current status). A hard DB error is returned as a non-nil error. -// A transition to RunStatusCancelled is refused with +// A transition to RunStatusCanceled is refused with // ErrLadderRunCancelViaTransition: cancels go through CancelLadderRun, which // stamps cancelled_by in the same UPDATE. func (s *PostgresStore) TransitionLadderRunStatus(ctx context.Context, id string, fromStatuses []ladder.RunStatus, toStatus ladder.RunStatus) (*LadderRunDB, error) { - if toStatus == ladder.RunStatusCancelled { + if toStatus == ladder.RunStatusCanceled { return nil, ErrLadderRunCancelViaTransition } // Typed public signature; the stringly-typed shape stays at the DB boundary. diff --git a/internal/config/store_postgres_ladder_amend_ceiling.go b/internal/config/store_postgres_ladder_amend_ceiling.go index 0a067f2e1..41d210324 100644 --- a/internal/config/store_postgres_ladder_amend_ceiling.go +++ b/internal/config/store_postgres_ladder_amend_ceiling.go @@ -109,7 +109,8 @@ type ladderCeilingSnapshot struct { } // loadLadderCeiling reads the latest non-canceled run's snapshot joined with the -// config's current coverage. It refuses a run without a full snapshot. +// config's current coverage. It refuses a run without a full snapshot. Rows +// keep either spelling of canceled (#1792), so both are excluded. func loadLadderCeiling(ctx context.Context, tx pgx.Tx, configID string) (ladderCeilingSnapshot, error) { var snap ladderCeilingSnapshot var hasSnapshot bool @@ -117,7 +118,7 @@ func loadLadderCeiling(ctx context.Context, tx pgx.Tx, configID string) (ladderC r.target_usd_hr IS NOT NULL AND r.existing_usd_hr IS NOT NULL AND r.baseline_usd_hr IS NOT NULL, COALESCE(trunc(LEAST(r.target_usd_hr,r.baseline_usd_hr*c.target_coverage/100)-r.existing_usd_hr,6),0)::float8 FROM ladder_runs r JOIN ladder_configs c ON c.id=r.config_id - WHERE r.config_id=$1 AND r.status<>'cancelled' ORDER BY r.started_at DESC,r.created_at DESC,r.id DESC LIMIT 1`, + WHERE r.config_id=$1 AND r.status NOT IN ('canceled','cancelled') ORDER BY r.started_at DESC,r.created_at DESC,r.id DESC LIMIT 1`, configID).Scan(&snap.anchor, &hasSnapshot, &snap.ceiling) if errors.Is(err, pgx.ErrNoRows) { return snap, fmt.Errorf("%w: config has no non-canceled run", ErrLadderAmendInvalid) diff --git a/internal/config/store_postgres_ladder_amend_ceiling_integration_test.go b/internal/config/store_postgres_ladder_amend_ceiling_integration_test.go index 4b841e4eb..9fe030f73 100644 --- a/internal/config/store_postgres_ladder_amend_ceiling_integration_test.go +++ b/internal/config/store_postgres_ladder_amend_ceiling_integration_test.go @@ -160,14 +160,19 @@ func TestLadderAmendCeiling_SnapshotAndCoverage(t *testing.T) { } func TestLadderAmendCeiling_LatestRunDecides(t *testing.T) { - f := newCeilingFixture(t) - _, ids := f.plan(fptr(10), fptr(0), fptr(100), 1, 1) - f.plan(fptr(3), fptr(0), fptr(100), 1) - require.ErrorIs(t, f.amend(ids[0], 2), ErrLadderAmendInvalid, "latest run's ceiling 3 < 4 in flight, older run's 10 must not apply") - runID, _ := f.plan(fptr(50), fptr(0), fptr(100)) - _, err := f.store.db.Exec(f.ctx, "UPDATE ladder_runs SET status='cancelled' WHERE id=$1", runID) - require.NoError(t, err) - require.ErrorIs(t, f.amend(ids[0], 2), ErrLadderAmendInvalid, "a canceled latest run is ignored") + // Both stored spellings of canceled must be skipped (#1792). + for _, canceled := range []string{"canceled", "cancelled"} { //nolint:misspell // legacy stored ladder status value + t.Run(canceled, func(t *testing.T) { + f := newCeilingFixture(t) + _, ids := f.plan(fptr(10), fptr(0), fptr(100), 1, 1) + f.plan(fptr(3), fptr(0), fptr(100), 1) + require.ErrorIs(t, f.amend(ids[0], 2), ErrLadderAmendInvalid, "latest run's ceiling 3 < 4 in flight, older run's 10 must not apply") + runID, _ := f.plan(fptr(50), fptr(0), fptr(100)) + _, err := f.store.db.Exec(f.ctx, "UPDATE ladder_runs SET status=$2 WHERE id=$1", runID, canceled) + require.NoError(t, err) + require.ErrorIs(t, f.amend(ids[0], 2), ErrLadderAmendInvalid, "a canceled latest run is ignored") + }) + } } func TestLadderAmendCeiling_WaitsForPlanner(t *testing.T) { @@ -266,7 +271,7 @@ func TestLadderAmendCeiling_CancelsOverdueBeforeCounting(t *testing.T) { require.NoError(t, f.amend(ids[1], 10), "X is not in flight, so 10 fits the ceiling") var status ladder.TrancheStatus require.NoError(t, f.store.db.QueryRow(f.ctx, "SELECT status FROM ladder_tranches WHERE id=$1", ids[0]).Scan(&status)) - require.Equal(t, ladder.TrancheStatusCancelled, status, "the overdue row must be canceled, not left for a claim with an older asOf") + require.Equal(t, ladder.TrancheStatusCanceled, status, "the overdue row must be canceled, not left for a claim with an older asOf") claimed, err := f.store.ClaimDueLadderTranches(f.ctx, f.cfgID, time.Now().Add(-time.Minute), 10, func(context.Context, pgx.Tx, LadderTrancheDB) (string, error) { return uuid.NewString(), nil }) require.NoError(t, err) diff --git a/internal/config/store_postgres_ladder_cancel.go b/internal/config/store_postgres_ladder_cancel.go index ee684b06d..e52635631 100644 --- a/internal/config/store_postgres_ladder_cancel.go +++ b/internal/config/store_postgres_ladder_cancel.go @@ -17,7 +17,7 @@ import ( var ErrLadderRunCancelActorRequired = errors.New("canceling a ladder run requires the actor's email") // ErrLadderRunCancelViaTransition is returned by TransitionLadderRunStatus for -// a transition to RunStatusCancelled: that path cannot stamp the canceled-by +// a transition to RunStatusCanceled: that path cannot stamp the canceled-by // audit column, so cancels must go through CancelLadderRun (issue #542). var ErrLadderRunCancelViaTransition = errors.New("ladder run cancel must use CancelLadderRun so the actor is recorded") @@ -50,7 +50,7 @@ func ladderRunStatusStrings(statuses []ladder.RunStatus) []string { } // CancelLadderRun atomically transitions a ladder_runs row from one of -// fromStatuses to RunStatusCancelled and stamps the canceled-by audit column +// fromStatuses to RunStatusCanceled and stamps the canceled-by audit column // with the actor's email in the same UPDATE. Returns the updated row on success, or (nil, nil) // when zero rows are affected (race lost or unexpected current status). An // empty actor is rejected with ErrLadderRunCancelActorRequired before any @@ -60,7 +60,7 @@ func (s *PostgresStore) CancelLadderRun(ctx context.Context, id string, fromStat if actor == "" { return nil, ErrLadderRunCancelActorRequired } - row := s.db.QueryRow(ctx, ladderRunCancelQuery, id, string(ladder.RunStatusCancelled), ladderRunStatusStrings(fromStatuses), actor) + row := s.db.QueryRow(ctx, ladderRunCancelQuery, id, string(ladder.RunStatusCanceled), ladderRunStatusStrings(fromStatuses), actor) result, err := scanLadderRun(row) if err != nil { if errors.Is(err, pgx.ErrNoRows) { diff --git a/internal/config/store_postgres_ladder_cancel_test.go b/internal/config/store_postgres_ladder_cancel_test.go index 2b0ca7c9a..debae71ef 100644 --- a/internal/config/store_postgres_ladder_cancel_test.go +++ b/internal/config/store_postgres_ladder_cancel_test.go @@ -39,9 +39,9 @@ func TestCancelLadderRun_StampsCancelledBy(t *testing.T) { now := time.Now().UTC() actor := "alice@example.com" pool.ExpectQuery(regexp.QuoteMeta(ladderRunCancelQuery)). - WithArgs("run-1", string(ladder.RunStatusCancelled), []string{"awaiting_approval", "approved"}, actor). + WithArgs("run-1", string(ladder.RunStatusCanceled), []string{"awaiting_approval", "approved"}, actor). WillReturnRows(pgxmock.NewRows(ladderRunMockColumns).AddRow( - "run-1", nil, now, nil, ladder.RunStatusCancelled, nil, nil, + "run-1", nil, now, nil, ladder.RunStatusCanceled, nil, nil, nil, nil, nil, nil, []byte(`{}`), 0.0, 0.0, 0.0, nil, nil, @@ -53,7 +53,7 @@ func TestCancelLadderRun_StampsCancelledBy(t *testing.T) { []ladder.RunStatus{ladder.RunStatusAwaitingApproval, ladder.RunStatusApproved}, " "+actor+" ") require.NoError(t, err) require.NotNil(t, run) - assert.Equal(t, ladder.RunStatusCancelled, run.Status) + assert.Equal(t, ladder.RunStatusCanceled, run.Status) require.NotNil(t, run.CancelledBy, "a canceled run must record who canceled it") assert.Equal(t, actor, *run.CancelledBy) require.NoError(t, pool.ExpectationsWereMet()) @@ -62,7 +62,7 @@ func TestCancelLadderRun_StampsCancelledBy(t *testing.T) { func TestCancelLadderRun_CASLostReturnsNil(t *testing.T) { store, pool := newLadderCancelMockStore(t) pool.ExpectQuery(regexp.QuoteMeta(ladderRunCancelQuery)). - WithArgs("run-1", string(ladder.RunStatusCancelled), []string{"approved"}, "bob@example.com"). + WithArgs("run-1", string(ladder.RunStatusCanceled), []string{"approved"}, "bob@example.com"). WillReturnRows(pgxmock.NewRows(ladderRunMockColumns)) run, err := store.CancelLadderRun(context.Background(), "run-1", @@ -86,7 +86,7 @@ func TestCancelLadderRun_RequiresActor(t *testing.T) { func TestTransitionLadderRunStatus_RefusesCancel(t *testing.T) { store, pool := newLadderCancelMockStore(t) run, err := store.TransitionLadderRunStatus(context.Background(), "run-1", - []ladder.RunStatus{ladder.RunStatusExecuting}, ladder.RunStatusCancelled) + []ladder.RunStatus{ladder.RunStatusExecuting}, ladder.RunStatusCanceled) require.ErrorIs(t, err, ErrLadderRunCancelViaTransition) assert.Nil(t, run) require.NoError(t, pool.ExpectationsWereMet(), "a cancel must not bypass the canceled-by stamp") diff --git a/internal/config/store_postgres_ladder_gate_integration_test.go b/internal/config/store_postgres_ladder_gate_integration_test.go index c45b01b4a..0e129ad30 100644 --- a/internal/config/store_postgres_ladder_gate_integration_test.go +++ b/internal/config/store_postgres_ladder_gate_integration_test.go @@ -57,7 +57,7 @@ func TestGetLadderExecutionGate(t *testing.T) { g, err := store.GetLadderExecutionGate(ctx, tr.ID) require.NoError(t, err) require.NotNil(t, g.RunStatus) - assert.Equal(t, string(ladder.RunStatusCancelled), *g.RunStatus) + assert.Equal(t, string(ladder.RunStatusCanceled), *g.RunStatus) }) t.Run("deleted config and run come back nil, not false", func(t *testing.T) { diff --git a/internal/config/store_postgres_ladder_lifecycle.go b/internal/config/store_postgres_ladder_lifecycle.go index 7fd703640..f0dadb582 100644 --- a/internal/config/store_postgres_ladder_lifecycle.go +++ b/internal/config/store_postgres_ladder_lifecycle.go @@ -30,7 +30,7 @@ func ladderGraceCutoff(asOf time.Time) time.Time { // unknown stays fired (and keeps netting) until it is resolved to completed or // failed. var ladderTrancheEdges = map[ladder.TrancheStatus][]ladder.TrancheStatus{ - ladder.TrancheStatusScheduled: {ladder.TrancheStatusFired, ladder.TrancheStatusCancelled}, + ladder.TrancheStatusScheduled: {ladder.TrancheStatusFired, ladder.TrancheStatusCanceled}, ladder.TrancheStatusFired: {ladder.TrancheStatusCompleted, ladder.TrancheStatusFailed}, } @@ -70,7 +70,7 @@ func (s *PostgresStore) TransitionLadderTrancheTx(ctx context.Context, tx pgx.Tx if from == ladder.TrancheStatusScheduled && to == ladder.TrancheStatusFired { return false, fmt.Errorf("ladder tranche %s: scheduled -> fired must go through ClaimDueLadderTranches", id) } - if reason == "" && (to == ladder.TrancheStatusFailed || to == ladder.TrancheStatusCancelled) { + if reason == "" && (to == ladder.TrancheStatusFailed || to == ladder.TrancheStatusCanceled) { return false, fmt.Errorf("ladder tranche %s: a %s transition needs a status reason", id, to) } tag, err := tx.Exec(ctx, ` diff --git a/internal/config/store_postgres_ladder_lifecycle_integration_test.go b/internal/config/store_postgres_ladder_lifecycle_integration_test.go index ae237fff8..a6294a3d2 100644 --- a/internal/config/store_postgres_ladder_lifecycle_integration_test.go +++ b/internal/config/store_postgres_ladder_lifecycle_integration_test.go @@ -125,7 +125,7 @@ func TestLadderGraceBoundary_ScheduledRowsAreCountedOrCancelledNeverNeither(t *t for _, r := range rows { got := trancheStatus(ctx, t, store, r.tr.ID) if r.sched { - canceled := got == ladder.TrancheStatusCancelled + canceled := got == ladder.TrancheStatusCanceled assert.NotEqual(t, r.counted, canceled, "scheduled row at %v: counted=%v status=%s (must be counted XOR canceled)", r.tr.ScheduledDate.Sub(asOf), r.counted, got) } else { @@ -225,8 +225,8 @@ func TestClaimDueLadderTranches_NeverClaimsOnDemandOrUnknownBasisRows(t *testing later := asOf.Add(LadderFireGrace) _, err = store.SaveLadderRunWithTranches(ctx, &LadderRunDB{ConfigID: &cfgID, StartedAt: later, Status: ladder.RunStatusPlanned}, nil) require.NoError(t, err) - assert.Equal(t, ladder.TrancheStatusCancelled, trancheStatus(ctx, t, store, unknown.ID), "past grace the sweep cancels it") - assert.Equal(t, ladder.TrancheStatusCancelled, trancheStatus(ctx, t, store, od.ID), "past grace the sweep cancels it") + assert.Equal(t, ladder.TrancheStatusCanceled, trancheStatus(ctx, t, store, unknown.ID), "past grace the sweep cancels it") + assert.Equal(t, ladder.TrancheStatusCanceled, trancheStatus(ctx, t, store, od.ID), "past grace the sweep cancels it") } func TestClaimDueLadderTranches_CallbackFailureRollsBackClaimAndExecution(t *testing.T) { @@ -281,10 +281,10 @@ func TestTransitionLadderTranche_EveryLegalEdgeAndCompareAndSet(t *testing.T) { require.NoError(t, store.SaveLadderTranches(ctx, []LadderTrancheDB{canceled, toCompleted, toFailed})) require.Equal(t, 7.0, inFlight(ctx, t, store, cfgID, asOf)) - ok, err := store.TransitionLadderTranche(ctx, canceled.ID, ladder.TrancheStatusScheduled, ladder.TrancheStatusCancelled, "test") + ok, err := store.TransitionLadderTranche(ctx, canceled.ID, ladder.TrancheStatusScheduled, ladder.TrancheStatusCanceled, "test") require.NoError(t, err) assert.True(t, ok) - ok, err = store.TransitionLadderTranche(ctx, canceled.ID, ladder.TrancheStatusScheduled, ladder.TrancheStatusCancelled, "test") + ok, err = store.TransitionLadderTranche(ctx, canceled.ID, ladder.TrancheStatusScheduled, ladder.TrancheStatusCanceled, "test") require.NoError(t, err) assert.False(t, ok, "second apply loses the compare-and-set") assert.Equal(t, 6.0, inFlight(ctx, t, store, cfgID, asOf), "canceled releases netting") @@ -293,7 +293,7 @@ func TestTransitionLadderTranche_EveryLegalEdgeAndCompareAndSet(t *testing.T) { require.NoError(t, err) assert.Equal(t, 6.0, inFlight(ctx, t, store, cfgID, asOf), "fired still nets") - ok, err = store.TransitionLadderTranche(ctx, toCompleted.ID, ladder.TrancheStatusScheduled, ladder.TrancheStatusCancelled, "test") + ok, err = store.TransitionLadderTranche(ctx, toCompleted.ID, ladder.TrancheStatusScheduled, ladder.TrancheStatusCanceled, "test") require.NoError(t, err) assert.False(t, ok, "a claimed tranche can no longer be canceled from scheduled") ok, err = store.TransitionLadderTranche(ctx, toCompleted.ID, ladder.TrancheStatusFired, ladder.TrancheStatusCompleted, "test") @@ -388,7 +388,7 @@ func TestClaimVsCancelAndOverdueSweep_ExactlyOneOutcome(t *testing.T) { }() go func() { defer wg.Done() - cancelWon, cancelErr = store.TransitionLadderTranche(ctx, tr.ID, ladder.TrancheStatusScheduled, ladder.TrancheStatusCancelled, "test") + cancelWon, cancelErr = store.TransitionLadderTranche(ctx, tr.ID, ladder.TrancheStatusScheduled, ladder.TrancheStatusCanceled, "test") }() go func() { defer wg.Done() @@ -404,7 +404,7 @@ func TestClaimVsCancelAndOverdueSweep_ExactlyOneOutcome(t *testing.T) { switch final { case ladder.TrancheStatusFired: assert.True(t, claimWon && !cancelWon, "iteration %d: fired but claimWon=%v cancelWon=%v", i, claimWon, cancelWon) - case ladder.TrancheStatusCancelled: + case ladder.TrancheStatusCanceled: assert.True(t, cancelWon && !claimWon, "iteration %d: canceled but claimWon=%v cancelWon=%v", i, claimWon, cancelWon) default: t.Fatalf("iteration %d: unexpected final status %s", i, final) diff --git a/internal/config/store_postgres_ladder_lifecycle_test.go b/internal/config/store_postgres_ladder_lifecycle_test.go index 5dc7c1c02..37a70db80 100644 --- a/internal/config/store_postgres_ladder_lifecycle_test.go +++ b/internal/config/store_postgres_ladder_lifecycle_test.go @@ -11,16 +11,16 @@ import ( var allLadderTrancheStatuses = []ladder.TrancheStatus{ ladder.TrancheStatusScheduled, ladder.TrancheStatusFired, ladder.TrancheStatusCompleted, - ladder.TrancheStatusCancelled, ladder.TrancheStatusFailed, + ladder.TrancheStatusCanceled, ladder.TrancheStatusFailed, } // Pure unit test (no database): the edge table is the whole state machine. func TestLadderTrancheEdges_OnlyDocumentedTransitionsAreLegal(t *testing.T) { legal := map[[2]ladder.TrancheStatus]bool{ - {ladder.TrancheStatusScheduled, ladder.TrancheStatusFired}: true, - {ladder.TrancheStatusScheduled, ladder.TrancheStatusCancelled}: true, - {ladder.TrancheStatusFired, ladder.TrancheStatusCompleted}: true, - {ladder.TrancheStatusFired, ladder.TrancheStatusFailed}: true, + {ladder.TrancheStatusScheduled, ladder.TrancheStatusFired}: true, + {ladder.TrancheStatusScheduled, ladder.TrancheStatusCanceled}: true, + {ladder.TrancheStatusFired, ladder.TrancheStatusCompleted}: true, + {ladder.TrancheStatusFired, ladder.TrancheStatusFailed}: true, } for _, from := range allLadderTrancheStatuses { for _, to := range allLadderTrancheStatuses { @@ -60,7 +60,7 @@ func TestTransitionLadderTranche_RequiresReasonForFailedAndCanceled(t *testing.T s := &PostgresStore{} for _, c := range []struct{ from, to ladder.TrancheStatus }{ {ladder.TrancheStatusFired, ladder.TrancheStatusFailed}, - {ladder.TrancheStatusScheduled, ladder.TrancheStatusCancelled}, + {ladder.TrancheStatusScheduled, ladder.TrancheStatusCanceled}, } { applied, err := s.TransitionLadderTrancheTx(context.Background(), nil, "id", c.from, c.to, "") assert.ErrorContains(t, err, "needs a status reason", "%s -> %s", c.from, c.to) diff --git a/internal/config/store_postgres_ladder_netting_integration_test.go b/internal/config/store_postgres_ladder_netting_integration_test.go index 9af689cd8..778395256 100644 --- a/internal/config/store_postgres_ladder_netting_integration_test.go +++ b/internal/config/store_postgres_ladder_netting_integration_test.go @@ -86,7 +86,7 @@ func TestLadderNetting_SelectionBoundaries(t *testing.T) { add(ladder.TrancheStatusCompleted, 32, lockTestAsOf, &anchor, "") add(ladder.TrancheStatusCompleted, 64, lockTestAsOf, &before, "") add(ladder.TrancheStatusFailed, 128, lockTestAsOf, nil, "") - add(ladder.TrancheStatusCancelled, 256, lockTestAsOf, nil, "") + add(ladder.TrancheStatusCanceled, 256, lockTestAsOf, nil, "") other := seedLadderConfigWithExtID(ctx, t, store, "fixture-other") require.NoError(t, store.SaveLadderTranches(ctx, []LadderTrancheDB{lifecycleTranche(other, 512, ladder.TrancheStatusFired, lockTestAsOf)})) _, err := store.PlanAndSaveLadderRun(ctx, cfg, lockTestAsOf, func(rows []LadderInFlightRow) (*LadderRunDB, []LadderTrancheDB, error) { @@ -95,7 +95,7 @@ func TestLadderNetting_SelectionBoundaries(t *testing.T) { }) require.NoError(t, err) var overdue int - require.NoError(t, store.db.QueryRow(ctx, "SELECT count(*) FROM ladder_tranches WHERE config_id=$1 AND status='cancelled' AND amount_usd_hr IN (4,8)", cfg).Scan(&overdue)) + require.NoError(t, store.db.QueryRow(ctx, "SELECT count(*) FROM ladder_tranches WHERE config_id=$1 AND status=$2 AND amount_usd_hr IN (4,8)", cfg, string(ladder.TrancheStatusCanceled)).Scan(&overdue)) require.Equal(t, 2, overdue) } diff --git a/internal/config/store_postgres_ladder_settled_integration_test.go b/internal/config/store_postgres_ladder_settled_integration_test.go index ddd264107..8b11994a1 100644 --- a/internal/config/store_postgres_ladder_settled_integration_test.go +++ b/internal/config/store_postgres_ladder_settled_integration_test.go @@ -57,7 +57,7 @@ func TestPostgresStoreDB_LadderSettledAt(t *testing.T) { _, err = store.TransitionLadderTranche(ctx, failed, ladder.TrancheStatusFired, ladder.TrancheStatusFailed, "boom") require.NoError(t, err) - _, err = store.TransitionLadderTranche(ctx, skipped, ladder.TrancheStatusScheduled, ladder.TrancheStatusCancelled, "no") + _, err = store.TransitionLadderTranche(ctx, skipped, ladder.TrancheStatusScheduled, ladder.TrancheStatusCanceled, "no") require.NoError(t, err) assert.Nil(t, settledAt(failed)) assert.Nil(t, settledAt(skipped)) @@ -106,7 +106,7 @@ func TestPostgresStoreDB_LadderSettledAt(t *testing.T) { add(ladder.TrancheStatusCompleted, 4, &after) assert.InDelta(t, 4, sum(), 1e-9, "one microsecond after counts") add(ladder.TrancheStatusFailed, 8, nil) - add(ladder.TrancheStatusCancelled, 16, nil) + add(ladder.TrancheStatusCanceled, 16, nil) add(ladder.TrancheStatusFired, 32, nil) add(ladder.TrancheStatusScheduled, 64, nil) assert.InDelta(t, 4, sum(), 1e-9, "only completed rows count") diff --git a/internal/config/store_postgres_ladder_settled_plan_integration_test.go b/internal/config/store_postgres_ladder_settled_plan_integration_test.go index 4cc26279f..715d0799c 100644 --- a/internal/config/store_postgres_ladder_settled_plan_integration_test.go +++ b/internal/config/store_postgres_ladder_settled_plan_integration_test.go @@ -55,7 +55,7 @@ func TestPostgresStoreDB_PlanAndSaveSettledTerm(t *testing.T) { add(ladder.TrancheStatusCompleted, 8, at(ladderSettleSkewMargin-time.Second)) assert.InDelta(t, 12, planned(), 1e-9, "settled within the skew margin before asOf counts") add(ladder.TrancheStatusFailed, 16, nil) - add(ladder.TrancheStatusCancelled, 32, nil) + add(ladder.TrancheStatusCanceled, 32, nil) assert.InDelta(t, 12, planned(), 1e-9, "failed and canceled never count") add(ladder.TrancheStatusFired, 64, nil) assert.InDelta(t, 76, planned(), 1e-9, "fired still counts as in-flight, added to the settled term") diff --git a/internal/config/store_postgres_ladder_test.go b/internal/config/store_postgres_ladder_test.go index 485535f40..47413a162 100644 --- a/internal/config/store_postgres_ladder_test.go +++ b/internal/config/store_postgres_ladder_test.go @@ -391,7 +391,7 @@ func TestPostgresStore_CancelLadderRun_StampsCancelledBy(t *testing.T) { require.NoError(t, err) refused, err := store.TransitionLadderRunStatus(ctx, run.ID, - []ladder.RunStatus{ladder.RunStatusAwaitingApproval}, ladder.RunStatusCancelled) + []ladder.RunStatus{ladder.RunStatusAwaitingApproval}, ladder.RunStatusCanceled) require.ErrorIs(t, err, ErrLadderRunCancelViaTransition) assert.Nil(t, refused) @@ -404,12 +404,12 @@ func TestPostgresStore_CancelLadderRun_StampsCancelledBy(t *testing.T) { []ladder.RunStatus{ladder.RunStatusAwaitingApproval}, "alice@example.com") require.NoError(t, err) require.NotNil(t, updated) - assert.Equal(t, ladder.RunStatusCancelled, updated.Status) + assert.Equal(t, ladder.RunStatusCanceled, updated.Status) got, err := store.GetLadderRun(ctx, run.ID) require.NoError(t, err) require.NotNil(t, got) - assert.Equal(t, ladder.RunStatusCancelled, got.Status) + assert.Equal(t, ladder.RunStatusCanceled, got.Status) require.NotNil(t, got.CancelledBy, "ladder_runs.cancelled_by must be stamped on cancel") assert.Equal(t, "alice@example.com", *got.CancelledBy) } @@ -575,7 +575,7 @@ func TestPostgresStore_InFlightLadderRows(t *testing.T) { insertTranche(t, &cfgID, 3.0, ladder.TrancheStatusScheduled) // included insertTranche(t, &cfgID, 2.0, ladder.TrancheStatusFired) // included (unresolved purchase) insertTranche(t, &cfgID, 5.0, ladder.TrancheStatusCompleted) // excluded (already in E) - insertTranche(t, &cfgID, 1.0, ladder.TrancheStatusCancelled) // excluded (terminal) + insertTranche(t, &cfgID, 1.0, ladder.TrancheStatusCanceled) // excluded (terminal) insertTranche(t, &cfgID, 4.0, ladder.TrancheStatusFailed) // excluded (terminal) // Still 'scheduled' but past the grace window: never fired, so never bought // (issue #118). Excluded, or past plans would net out of every later gap. @@ -731,12 +731,12 @@ func TestPostgresStore_SaveLadderRunWithTranches_CancelsOverdue(t *testing.T) { // 12 day-0 tranches + 10 two-week tranches past the grace window; the // 11th is due exactly at start (within grace, still netted/fireable) and // the 12th is in the future. - assert.Equal(t, 22, countStatus(t, cfgID, ladder.TrancheStatusCancelled), "overdue scheduled rows must be marked canceled, not deleted") + assert.Equal(t, 22, countStatus(t, cfgID, ladder.TrancheStatusCanceled), "overdue scheduled rows must be marked canceled, not deleted") assert.Equal(t, 1, countStatus(t, cfgID, ladder.TrancheStatusFired), "an overdue fired row must be left alone") assert.Equal(t, 1, countStatus(t, cfgID, ladder.TrancheStatusCompleted), "an overdue completed row must be left alone") var linkedStatus string require.NoError(t, store.db.QueryRow(ctx, `SELECT status FROM ladder_tranches WHERE id = $1`, linked.ID).Scan(&linkedStatus)) assert.Equal(t, string(ladder.TrancheStatusScheduled), linkedStatus, "an overdue row linked to an execution must be left alone") assert.Equal(t, 1, countStatus(t, otherID, ladder.TrancheStatusScheduled), "another config's tranches must be untouched") - assert.Equal(t, 0, countStatus(t, otherID, ladder.TrancheStatusCancelled)) + assert.Equal(t, 0, countStatus(t, otherID, ladder.TrancheStatusCanceled)) } diff --git a/internal/config/suppressions_build.go b/internal/config/suppressions_build.go index 5051cb31c..439aac763 100644 --- a/internal/config/suppressions_build.go +++ b/internal/config/suppressions_build.go @@ -34,7 +34,7 @@ func suppressionBucketOf(rec *RecommendationRecord) suppressionBucket { service: CanonicalSuppressionService(rec.Provider, rec.Service), region: rec.Region, resourceType: rec.ResourceType, - engine: rec.Engine, + engine: CanonicalSuppressionEngine(rec.Provider, CanonicalSuppressionService(rec.Provider, rec.Service), rec.Engine), } v, err := rec.SuppressionVariant() if err != nil { diff --git a/internal/config/suppressions_build_test.go b/internal/config/suppressions_build_test.go index 2215f404f..bf23018ec 100644 --- a/internal/config/suppressions_build_test.go +++ b/internal/config/suppressions_build_test.go @@ -340,3 +340,17 @@ func TestCanonicalSuppressionTenancy(t *testing.T) { assert.Equal(t, "Dedicated", canonicalSuppressionTenancy("Dedicated")) assert.Equal(t, "default", canonicalSuppressionTenancy("default")) } + +func TestCanonicalSuppressionEngine(t *testing.T) { + for _, tc := range []struct{ provider, service, engine, want string }{ + {"azure", "cache", "", "redis"}, + {"azure", "cache", "redis", "redis"}, + {"azure", "cache", "valkey", "valkey"}, + {"aws", "cache", "", ""}, + {"azure", "compute", "", ""}, + } { + assert.Equal(t, tc.want, CanonicalSuppressionEngine(tc.provider, tc.service, tc.engine), tc.provider+"/"+tc.service+"/"+tc.engine) + } + assert.Equal(t, []string{"redis", ""}, SuppressionEngineSpellings("azure", "cache", "redis")) + assert.Equal(t, []string{""}, SuppressionEngineSpellings("aws", "cache", "")) +} diff --git a/internal/config/types.go b/internal/config/types.go index e55729b5c..7f964cccd 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -860,6 +860,28 @@ func CanonicalSuppressionService(provider, service string) string { return service } +// CanonicalSuppressionEngine reads an Azure cache engine "" as "redis" so a +// suppression and a rec match across go#702, which gave Advisor-sourced Azure +// Redis recs an engine (they had none before). Other tuples are unchanged. +func CanonicalSuppressionEngine(provider, service, engine string) string { + if provider == string(common.ProviderAzure) && service == string(common.ServiceCache) && engine == "" { + return "redis" + } + return engine +} + +// StoredRecommendationService is the service slug the platform stores and keys +// a provider recommendation under. go#702 relabelled Azure Redis recs from +// cache to memorydb; the platform's IDs, suppressions and per-account config +// key Azure Redis as cache, so it maps back here. AWS memorydb is a distinct +// service and is unchanged. +func StoredRecommendationService(provider string, service common.ServiceType) string { + if provider == string(common.ProviderAzure) && service == common.ServiceMemoryDB { + return string(common.ServiceCache) + } + return string(service) +} + // AppliesTo reports whether the suppression counts against a recommendation // on the same 6-tuple whose variant is v. A row without a variant (legacy) // applies to every variant, and a nil v (the rec's Details could not be read) diff --git a/internal/config/types_test.go b/internal/config/types_test.go index 99ea86a1a..c43101b22 100644 --- a/internal/config/types_test.go +++ b/internal/config/types_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -494,3 +495,23 @@ func TestIsUSDCommitmentCurrency(t *testing.T) { } } } + +func TestStoredRecommendationService(t *testing.T) { + cases := []struct { + provider string + service common.ServiceType + want string + }{ + {"azure", common.ServiceMemoryDB, "cache"}, + {"azure", common.ServiceCache, "cache"}, + {"azure", common.ServiceCompute, string(common.ServiceCompute)}, + {"aws", common.ServiceMemoryDB, "memorydb"}, + {"aws", common.ServiceCache, "cache"}, + {"aws", common.ServiceEC2, "ec2"}, + {"gcp", common.ServiceMemoryDB, "memorydb"}, + {"gcp", common.ServiceCache, "cache"}, + } + for _, c := range cases { + assert.Equal(t, c.want, StoredRecommendationService(c.provider, c.service), "%s/%s", c.provider, c.service) + } +} diff --git a/internal/purchase/azure_redis_routing_test.go b/internal/purchase/azure_redis_routing_test.go new file mode 100644 index 000000000..91a8cb0e6 --- /dev/null +++ b/internal/purchase/azure_redis_routing_test.go @@ -0,0 +1,70 @@ +package purchase + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// #1801: only the managedredis client (ServiceMemoryDB) buys Azure Redis +// reservations; the Azure cache client refuses every purchase +// (cloud-commitments-go#694). A recommendation labeled "cache" must reach the +// right client for its provider, and keep its own label everywhere else. +func purchaseCacheRecThrough(t *testing.T, prov *MockProvider, providerName, region string, want common.ServiceType) { + t.Helper() + ctx := context.Background() + store, sender, factory, svc := new(MockConfigStore), new(MockEmailSender), new(MockProviderFactory), new(MockServiceClient) + assertMocksOnCleanup(t, store, sender, factory, prov, svc) + store.SavePurchaseExecutionFn = func(context.Context, *config.PurchaseExecution) error { return nil } + store.On("SavePurchaseHistory", ctx, mock.Anything).Return(nil) + factory.On("CreateAndValidateProvider", mock.Anything, providerName, mock.Anything).Return(prov, nil) + prov.On("GetServiceClient", mock.Anything, want, region).Return(provider.ServiceClient(svc), nil) + svc.On("PurchaseCommitment", mock.Anything, mock.MatchedBy(func(rec common.Recommendation) bool { + return rec.Service == common.ServiceCache + }), mock.Anything).Return(common.PurchaseResult{Success: true, CommitmentID: "order-1"}, nil) + + exec := &config.PurchaseExecution{ExecutionID: "exec-1801-" + providerName, IdempotencyKey: "lineage-1801-" + providerName, + Recommendations: []config.RecommendationRecord{{ID: "rec-1", Provider: providerName, Service: "cache", + ResourceType: "cache.r6g.large", Region: region, Count: 1, Term: 1, Selected: true}}} + m := &Manager{config: store, email: sender, providerFactory: factory} + _, _, errs, err := m.processPurchaseRecommendations(ctx, exec, &config.PurchasePlan{Name: "p"}, "acct", (*provider.Config)(nil), "") + require.NoError(t, err) + require.Empty(t, errs) + require.True(t, exec.Recommendations[0].Purchased, "the purchase must reach the %s client", want) +} + +// [fail-first] On main the Azure lookup asks for ServiceCache, which this +// mock provider does not serve, so the purchase fails. +func TestAzureCachePurchaseUsesTheManagedRedisClient(t *testing.T) { + prov := new(MockProvider) + purchaseCacheRecThrough(t, prov, "azure", "westeurope", common.ServiceMemoryDB) + prov.AssertNotCalled(t, "GetServiceClient", mock.Anything, common.ServiceCache, mock.Anything) +} + +// [guard] AWS "cache" stays on the cache (ElastiCache) client. +func TestAWSCachePurchaseKeepsTheCacheClient(t *testing.T) { + prov := new(MockProvider) + purchaseCacheRecThrough(t, prov, "aws", "us-east-1", common.ServiceCache) + prov.AssertNotCalled(t, "GetServiceClient", mock.Anything, common.ServiceMemoryDB, mock.Anything) +} + +func TestPurchaseClientService(t *testing.T) { + for _, tc := range []struct { + provider string + in, want common.ServiceType + }{ + {"azure", common.ServiceCache, common.ServiceMemoryDB}, + {"azure", common.ServiceMemoryDB, common.ServiceMemoryDB}, + {"azure", common.ServiceCompute, common.ServiceCompute}, + {"aws", common.ServiceCache, common.ServiceCache}, + {"gcp", common.ServiceCache, common.ServiceCache}, + {"", common.ServiceCache, common.ServiceCache}, + } { + require.Equal(t, tc.want, purchaseClientService(tc.provider, tc.in), "%s/%s", tc.provider, tc.in) + } +} diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index 44d777e37..fe1a09f1f 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -27,7 +27,7 @@ var serverNotePatterns = []string{ `plan step refused by commitment cap(?:: plan has no max_step_commitment set \(auto-purchase of steps with recommendations is refused until one is set\)|: step total commitment \d+\.\d{2} exceeds max_step_commitment \d+\.\d{2})?`, `scheduled purchase skipped: it was due too long ago(?:: it was due at \d{4}-\d{2}-\d{2}T[0-9:.]+Z, [0-9a-z.]+ ago, which is more than the [0-9a-z.]+ limit \(SCHEDULED_PURCHASE_MAX_OVERDUE\); nothing was submitted, retry it if the purchase is still wanted)?`, `purchase refused after claim: the claimed execution no longer passes the authorization checks`, - `refusing to execute ladder purchase: (?:global config unavailable|ladder planning is switched off|ladder execution is switched off|its tranche status is unknown|its tranche is [a-z_]+|its ladder config no longer exists|its ladder config is disabled|ladder config mode is "[a-z_]*", not auto_approve|its ladder run no longer exists|its ladder run was canceled)`, + `refusing to execute ladder purchase: (?:global config unavailable|ladder planning is switched off|ladder execution is switched off|its tranche status is unknown|its tranche is [a-z_]+|its ladder config no longer exists|its ladder config is disabled|ladder config mode is "[a-z_]*", not auto_approve|its ladder run no longer exists|its ladder run was canceled|its ladder run status "[a-z_]{1,32}" is unknown)`, `scheduled plan step reached its scheduled date without recommendations; no purchase was submitted`, `no current recommendation matched plan ` + uuidPattern + ` step \d+ by \d{4}-\d{2}-\d{2} \(\d+-day grace after the plan date\)(?:; ` + skippedNotePattern + `)?`, `target already met: target \d+, committed \d+(?:; ` + skippedNotePattern + `)?; \d+ failed row\(s\) of unknown outcome counted as committed`, diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index d2b430f6f..9e40c15fd 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -39,6 +39,7 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { dueRowAgeRefusal(&config.PurchaseExecution{ScheduledDate: time.Now().Add(-100 * time.Hour)}, time.Hour, time.Now()).Error(), ErrClaimedRowRefused.Error(), "refusing to execute ladder purchase: its ladder run was canceled", + `refusing to execute ladder purchase: its ladder run status "weird_state" is unknown`, "refusing to execute ladder purchase: its tranche is paused", "refusing to execute: ladder tranche lookup failed", "refusing to execute ladder purchase: global config unreadable", @@ -235,8 +236,11 @@ func TestSanitizeStoredError_SlotsRejectWhitespaceAndControls(t *testing.T) { errAmbiguousAccountScope.Error() + ": 1 selected recommendation(s) carry no cloud_account_id while 2 target account %s", "EC2 recommendation for %s has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation", `refusing to execute retry attempt 1: provider "%s" is not known to reject a duplicate purchase, so re-driving this could buy a second commitment`, + `refusing to execute ladder purchase: its ladder run status "%s" is unknown`, resolve, } + quoted := `refusing to execute ladder purchase: its ladder run status "a\"b" is unknown` + assert.NotEqual(t, quoted, SanitizeStoredError(quoted)) for _, bad := range []string{"a b", "a\nb", "a\rb", "a\tb", "a\vb", "a\fb", "a\u0085b", "a\u00a0b", "a\u2028b", "a\u200bb", "a\x00b"} { for _, slot := range slots { if slot == resolve && bad == "a b" { diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 1965e0f85..15e98de59 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -1278,7 +1278,7 @@ func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.Recommen // Get the service client for this region t0 = time.Now() - serviceClient, err := cloudProvider.GetServiceClient(recCtx, serviceType, rec.Region) + serviceClient, err := cloudProvider.GetServiceClient(recCtx, purchaseClientService(rec.Provider, serviceType), rec.Region) if err != nil { logging.Errorf("purchase[%s]: service client lookup failed for %s/%s in %s after %s: %v", opts.ExecutionID, rec.Provider, serviceType, rec.Region, time.Since(t0), err) @@ -1417,6 +1417,18 @@ func resolveEC2OfferingClass(s string) (ec2types.OfferingClassType, error) { } } +// purchaseClientService is the service whose client buys a recommendation of +// service. Azure Redis recommendations are labeled ServiceCache, but only the +// managedredis client (ServiceMemoryDB) buys RedisCache reservations; the +// Azure cache client refuses every purchase (cloud-commitments-go#694, #1801). +// Labels in history and suppressions keep the recommendation's own service. +func purchaseClientService(providerName string, service common.ServiceType) common.ServiceType { + if providerName == string(common.ProviderAzure) && service == common.ServiceCache { + return common.ServiceMemoryDB + } + return service +} + // mapServiceSlug returns the common.ServiceType for a canonical or legacy // service slug. // diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go index eed8fc74a..bd7714b5a 100644 --- a/internal/purchase/execution_test.go +++ b/internal/purchase/execution_test.go @@ -1449,10 +1449,12 @@ func TestExecutePurchase_AzureCanonicalServiceTypes(t *testing.T) { expectedType: common.ServiceCompute, }, { - name: "cache_canonical", - service: "cache", - resource: "Standard_C1", - expectedType: common.ServiceCache, + name: "cache_canonical", + service: "cache", + resource: "Standard_C1", + // Azure Redis is bought by the managedredis client; the cache + // client refuses purchases (cloud-commitments-go#694, #1801). + expectedType: common.ServiceMemoryDB, }, { name: "relational_db_canonical", diff --git a/internal/purchase/ladder_execution_recheck.go b/internal/purchase/ladder_execution_recheck.go index 44c29b82e..21ca8a161 100644 --- a/internal/purchase/ladder_execution_recheck.go +++ b/internal/purchase/ladder_execution_recheck.go @@ -74,7 +74,18 @@ func ladderGateRefusal(gate *config.LadderExecutionGate) error { return fmt.Errorf("refusing to execute ladder purchase: ladder config mode is %q, not auto_approve", *gate.ConfigMode) case gate.RunStatus == nil: return errors.New("refusing to execute ladder purchase: its ladder run no longer exists") - case ladder.RunStatus(*gate.RunStatus) == ladder.RunStatusCancelled: + } + return ladderRunRefusal(*gate.RunStatus) +} + +// ladderRunRefusal parses the stored run status, so both stored spellings of +// canceled refuse (#1792), and a status the code does not know fails closed. +func ladderRunRefusal(status string) error { + run, err := ladder.ParseRunStatus(status) + if err != nil { + return fmt.Errorf("refusing to execute ladder purchase: its ladder run status %q is unknown", status) + } + if run == ladder.RunStatusCanceled { return errors.New("refusing to execute ladder purchase: its ladder run was canceled") } return nil diff --git a/internal/purchase/ladder_execution_recheck_test.go b/internal/purchase/ladder_execution_recheck_test.go index 8a640b6e2..aa3f2382e 100644 --- a/internal/purchase/ladder_execution_recheck_test.go +++ b/internal/purchase/ladder_execution_recheck_test.go @@ -80,12 +80,15 @@ func TestLadderRecheck_RefusesBeforeProviderCall(t *testing.T) { {"planning flag off", openGate(), nil, ladderGlobal(false, true), nil, "planning is switched off"}, {"config disabled", gateWith(func(g *config.LadderExecutionGate) { g.ConfigEnabled = ptr(false) }), nil, ladderGlobal(true, true), nil, "config is disabled"}, {"config mode email_approval", gateWith(func(g *config.LadderExecutionGate) { g.ConfigMode = ptr("email_approval") }), nil, ladderGlobal(true, true), nil, "not auto_approve"}, - {"run canceled", gateWith(func(g *config.LadderExecutionGate) { g.RunStatus = ptr(string(ladder.RunStatusCancelled)) }), nil, ladderGlobal(true, true), nil, "run was canceled"}, + {"run canceled", gateWith(func(g *config.LadderExecutionGate) { g.RunStatus = ptr("canceled") }), nil, ladderGlobal(true, true), nil, "run was canceled"}, + {"run cancelled (legacy row)", gateWith(func(g *config.LadderExecutionGate) { g.RunStatus = ptr("cancelled") }), nil, ladderGlobal(true, true), nil, "run was canceled"}, //nolint:misspell // legacy stored ladder status value + {"run status not known to the code", gateWith(func(g *config.LadderExecutionGate) { g.RunStatus = ptr("bogus") }), nil, ladderGlobal(true, true), nil, `run status "bogus" is unknown`}, {"config deleted", gateWith(func(g *config.LadderExecutionGate) { g.ConfigEnabled, g.ConfigMode = nil, nil }), nil, ladderGlobal(true, true), nil, "config no longer exists"}, {"run deleted", gateWith(func(g *config.LadderExecutionGate) { g.RunStatus = nil }), nil, ladderGlobal(true, true), nil, "run no longer exists"}, {"tranche failed by operator", gateWith(func(g *config.LadderExecutionGate) { g.TrancheStatus = ptr(string(ladder.TrancheStatusFailed)) }), nil, ladderGlobal(true, true), nil, "its tranche is failed"}, {"tranche completed", gateWith(func(g *config.LadderExecutionGate) { g.TrancheStatus = ptr(string(ladder.TrancheStatusCompleted)) }), nil, ladderGlobal(true, true), nil, "its tranche is completed"}, - {"tranche cancelled", gateWith(func(g *config.LadderExecutionGate) { g.TrancheStatus = ptr(string(ladder.TrancheStatusCancelled)) }), nil, ladderGlobal(true, true), nil, "its tranche is cancelled"}, //nolint:misspell // "cancelled" is the stored ladder status value + {"tranche cancelled (legacy row)", gateWith(func(g *config.LadderExecutionGate) { g.TrancheStatus = ptr("cancelled") }), nil, ladderGlobal(true, true), nil, "its tranche is cancelled"}, //nolint:misspell // legacy stored ladder status value + {"tranche canceled", gateWith(func(g *config.LadderExecutionGate) { g.TrancheStatus = ptr("canceled") }), nil, ladderGlobal(true, true), nil, "its tranche is canceled"}, {"tranche status not known to the code", gateWith(func(g *config.LadderExecutionGate) { g.TrancheStatus = ptr("bogus") }), nil, ladderGlobal(true, true), nil, "its tranche is bogus"}, {"tranche status unknown", gateWith(func(g *config.LadderExecutionGate) { g.TrancheStatus = nil }), nil, ladderGlobal(true, true), nil, "tranche status is unknown"}, {"tranche lookup DB error", nil, errors.New("connection reset"), ladderGlobal(true, true), nil, "tranche lookup failed"}, diff --git a/internal/scheduler/azure_redis_service_test.go b/internal/scheduler/azure_redis_service_test.go new file mode 100644 index 000000000..4316a7db5 --- /dev/null +++ b/internal/scheduler/azure_redis_service_test.go @@ -0,0 +1,130 @@ +package scheduler + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/managedredis" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func redisRec(provider common.ProviderType, service common.ServiceType, account string) common.Recommendation { + return common.Recommendation{ + Provider: provider, Account: account, Service: service, Region: "westeurope", + ResourceType: "C1", Count: 3, Term: "1yr", PaymentOption: "all-upfront", + } +} + +// advisorRedisRecs returns the Advisor-sourced Azure Redis rec in its real +// pre-bump shape (cache, no Details) and post-bump shape (memorydb with +// managedredis.Details, go#702 providers/azure/recommendations.go:654-656). +func advisorRedisRecs() (before, after common.Recommendation) { + before = redisRec(common.ProviderAzure, common.ServiceCache, "sub-1") + after = redisRec(common.ProviderAzure, common.ServiceMemoryDB, "sub-1") + after.Details = managedredis.Details(after.ResourceType) + return before, after +} + +// go#702 emits Azure Redis as memorydb; the Consumption rec keeps its former +// cache ID, and an Advisor-only rec shifts ID once because it gains engine +// "redis" (accepted; suppressions are engine-canonicalised instead). +func TestConvertRecommendations_AzureMemoryDBStoredAsCache(t *testing.T) { + s := &Scheduler{} + consumption := redisRec(common.ProviderAzure, common.ServiceMemoryDB, "sub-1") + consumption.Details = managedredis.Details(consumption.ResourceType) + _, advisor := advisorRedisRecs() + want := s.convertRecommendations([]common.Recommendation{consumption}, "azure") + got := s.convertRecommendations([]common.Recommendation{advisor}, "azure") + require.Len(t, want, 1) + require.Len(t, got, 1) + assert.Equal(t, "cache", got[0].Service) + assert.Equal(t, "redis", got[0].Engine) + assert.Equal(t, want[0].ID, got[0].ID) +} + +func TestApplySuppressions_AdvisorRedisEngineStableAcrossGoBump(t *testing.T) { + s := &Scheduler{} + before, after := advisorRedisRecs() + oldRec := s.convertRecommendations([]common.Recommendation{before}, "azure")[0] + newRec := s.convertRecommendations([]common.Recommendation{after}, "azure")[0] + require.Equal(t, "", oldRec.Engine) + require.Equal(t, "redis", newRec.Engine) + for _, tc := range []struct { + name string + supEngine string + rec config.RecommendationRecord + }{ + {"old suppression hides new rec", oldRec.Engine, newRec}, + {"new suppression hides old rec", newRec.Engine, oldRec}, + {"same-engine control", newRec.Engine, newRec}, + } { + rec := tc.rec + rec.CloudAccountID = strPtr("acct-1") + store := &mockSuppressionStore{ + recs: []config.RecommendationRecord{rec}, + sups: []config.PurchaseSuppression{{ + ExecutionID: "e1", AccountID: "acct-1", Provider: "azure", Service: "cache", + Region: rec.Region, ResourceType: rec.ResourceType, Engine: tc.supEngine, + SuppressedCount: 1, ExpiresAt: time.Now().Add(24 * time.Hour), + }}, + } + got, err := (&Scheduler{config: store}).ListRecommendations(context.Background(), config.RecommendationFilter{}) + require.NoError(t, err, tc.name) + require.Len(t, got, 1, tc.name) + assert.Equal(t, rec.Count-1, got[0].Count, tc.name) + } +} + +func TestConvertRecommendations_AWSMemoryDBUnchanged(t *testing.T) { + s := &Scheduler{} + got := s.convertRecommendations([]common.Recommendation{redisRec(common.ProviderAWS, common.ServiceMemoryDB, "")}, "aws") + require.Len(t, got, 1) + assert.Equal(t, "memorydb", got[0].Service) + assert.Equal(t, "aws||memorydb|westeurope|C1||1|all-upfront", got[0].ID) +} + +func azureMemoryDBStored(t *testing.T, acct string) config.RecommendationRecord { + t.Helper() + s := &Scheduler{} + out := s.convertRecommendations([]common.Recommendation{redisRec(common.ProviderAzure, common.ServiceMemoryDB, "")}, "azure") + require.Len(t, out, 1) + out[0].CloudAccountID = strPtr(acct) + return out[0] +} + +// The no-re-buy case: a suppression written under azure/cache still hides a +// memorydb-sourced Azure rec through the real ListRecommendations path. +func TestApplySuppressions_AzureCacheSuppressionHidesMemoryDBSourcedRec(t *testing.T) { + rec := azureMemoryDBStored(t, "acct-1") + store := &mockSuppressionStore{ + recs: []config.RecommendationRecord{rec}, + sups: []config.PurchaseSuppression{{ + ExecutionID: "e1", AccountID: "acct-1", Provider: "azure", Service: "cache", + Region: rec.Region, ResourceType: rec.ResourceType, SuppressedCount: rec.Count, + ExpiresAt: time.Now().Add(24 * time.Hour), + }}, + } + s := &Scheduler{config: store} + got, err := s.ListRecommendations(context.Background(), config.RecommendationFilter{}) + require.NoError(t, err) + assert.Empty(t, got, "fully suppressed rec must not be offered again") +} + +// azure/cache per-account toggles still apply to a memorydb-sourced rec. +func TestFilterRecsByResolvedConfigs_AzureCacheConfigAppliesToMemoryDBSourcedRec(t *testing.T) { + rec := azureMemoryDBStored(t, "acct-1") + key := config.AccountConfigKey("acct-1", "azure", "cache") + for name, cfg := range map[string]*config.ServiceConfig{ + "disabled": {Enabled: false}, + "min_count": {Enabled: true, MinCount: rec.Count + 1}, + } { + got := filterRecsByResolvedConfigs([]config.RecommendationRecord{rec}, map[string]*config.ServiceConfig{key: cfg}) + assert.Empty(t, got, name) + } + got := filterRecsByResolvedConfigs([]config.RecommendationRecord{rec}, map[string]*config.ServiceConfig{key: {Enabled: true, MinCount: rec.Count}}) + assert.Len(t, got, 1) +} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 3d1b89525..964386b81 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1517,8 +1517,9 @@ func aggregateSuppressions(sups []config.PurchaseSuppression) map[suppressionKey k := suppressionKey{ accountID: sup.AccountID, provider: sup.Provider, service: config.CanonicalSuppressionService(sup.Provider, sup.Service), - region: sup.Region, resourceType: sup.ResourceType, engine: sup.Engine, + region: sup.Region, resourceType: sup.ResourceType, } + k.engine = config.CanonicalSuppressionEngine(k.provider, k.service, sup.Engine) index[k] = append(index[k], sup) } return index @@ -1563,8 +1564,9 @@ func applySuppressionIndex(recs []config.RecommendationRecord, index map[suppres k := suppressionKey{ accountID: accountID, provider: rec.Provider, service: config.CanonicalSuppressionService(rec.Provider, rec.Service), - region: rec.Region, resourceType: rec.ResourceType, engine: rec.Engine, + region: rec.Region, resourceType: rec.ResourceType, } + k.engine = config.CanonicalSuppressionEngine(k.provider, k.service, rec.Engine) a := matchingSuppressions(&rec, index[k]) if a == nil { out = append(out, rec) @@ -1824,7 +1826,7 @@ func (s *Scheduler) convertRecommendations(recs []common.Recommendation, provide record := config.RecommendationRecord{ SourceAccountID: sourceAccountID, Provider: providerName, - Service: string(rec.Service), + Service: config.StoredRecommendationService(providerName, rec.Service), Region: rec.Region, ResourceType: rec.ResourceType, Engine: engine, diff --git a/internal/scheduler/scheduler_page.go b/internal/scheduler/scheduler_page.go index eae378f99..2b6c1dc85 100644 --- a/internal/scheduler/scheduler_page.go +++ b/internal/scheduler/scheduler_page.go @@ -135,7 +135,9 @@ func suppressionPageKeys(index map[suppressionKey][]config.PurchaseSuppression) var keys []string for k := range index { for _, service := range config.SuppressionServiceSpellings(k.provider, k.service) { - keys = append(keys, config.SuppressionTupleKey(k.accountID, k.provider, service, k.region, k.resourceType, k.engine)) + for _, engine := range config.SuppressionEngineSpellings(k.provider, k.service, k.engine) { + keys = append(keys, config.SuppressionTupleKey(k.accountID, k.provider, service, k.region, k.resourceType, engine)) + } } } sort.Strings(keys) diff --git a/internal/server/handler_ladder_immediate_integration_test.go b/internal/server/handler_ladder_immediate_integration_test.go index 521c00f4e..34ac67bbf 100644 --- a/internal/server/handler_ladder_immediate_integration_test.go +++ b/internal/server/handler_ladder_immediate_integration_test.go @@ -202,8 +202,8 @@ func TestLadderB1_NextRunNetsImmediateRowWithinGrace(t *testing.T) { }{ {"1h", time.Hour, 0, string(pkgladder.TrancheStatusScheduled)}, {"grace minus 1us", config.LadderFireGrace - time.Microsecond, 0, string(pkgladder.TrancheStatusScheduled)}, - {"exactly grace", config.LadderFireGrace, 5, string(pkgladder.TrancheStatusCancelled)}, - {"grace plus 1us", config.LadderFireGrace + time.Microsecond, 5, string(pkgladder.TrancheStatusCancelled)}, + {"exactly grace", config.LadderFireGrace, 5, string(pkgladder.TrancheStatusCanceled)}, + {"grace plus 1us", config.LadderFireGrace + time.Microsecond, 5, string(pkgladder.TrancheStatusCanceled)}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) {