diff --git a/frontend/src/__tests__/ladder.test.ts b/frontend/src/__tests__/ladder.test.ts new file mode 100644 index 000000000..307e71a7f --- /dev/null +++ b/frontend/src/__tests__/ladder.test.ts @@ -0,0 +1,136 @@ +/** + * Commitment Laddering settings module tests (issue #1333 phase 3). + * + * Covers the two security/correctness-sensitive surfaces: + * (a) renderConfigTable escapes attacker-controlled cloud_account_id/provider + * so an injected tag never lands raw in innerHTML (XSS). + * (b) saveLadderConfig's max_hourly_commit_per_run guard: blank -> null, + * positive -> passthrough, non-positive/invalid -> rejected (no store call). + */ +import { initLadderingSettings, renderConfigTable, saveLadderConfig } from '../ladder'; +import type { LadderConfig } from '../api'; + +jest.mock('../api', () => ({ + getLadderConfigs: jest.fn(), + upsertLadderConfig: jest.fn(), + getConfig: jest.fn(), + updateConfig: jest.fn(), +})); + +jest.mock('../permissions', () => ({ + canAccess: jest.fn(() => true), +})); + +const mockShowToast = jest.fn(); +jest.mock('../toast', () => ({ + showToast: (opts: unknown) => mockShowToast(opts), +})); + +import * as api from '../api'; + +function baseConfig(overrides: Partial = {}): LadderConfig { + return { + cloud_account_id: '11111111-1111-1111-1111-111111111111', + provider: 'aws', + enabled: true, + mode: 'email_approval', + cadence: 'daily', + target_coverage: 100, + buffer_fraction: 0.1, + baseline_percentile: 5, + lookback_days: 30, + buffer_utilization_threshold: 90, + max_hourly_commit_per_run: null, + max_actions_per_run: 10, + ramp_schedule: { steps: [{ after_days: 0, fraction: 1.0 }] }, + updated_at: '2026-01-15T10:00:00Z', + ...overrides, + } as LadderConfig; +} + +// Render the full section (incl. the modal form) into the DOM so the exported +// helpers have the elements they read. +async function renderSection(): Promise { + document.body.innerHTML = '
'; + (api.getLadderConfigs as jest.Mock).mockResolvedValue([]); + await initLadderingSettings(false); +} + +describe('ladder.ts', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockShowToast.mockReset(); + }); + + describe('renderConfigTable XSS escaping', () => { + test('renders a malicious cloud_account_id/provider as inert text, not markup', async () => { + await renderSection(); + const evil = ''; + renderConfigTable([baseConfig({ cloud_account_id: evil, provider: evil })]); + + const container = document.getElementById('ladder-configs-table-container')!; + + // The injected payload must not create a live element anywhere in the + // table (DOM-level assertion; reading serialized innerHTML is unreliable + // because jsdom re-serializes attribute values with raw which are + // inert inside a quoted attribute). + expect(container.querySelector('img')).toBeNull(); + + // The account-id cell holds the payload as TEXT (no child elements => + // it was escaped, not parsed as HTML). + const firstCell = container.querySelector('tbody tr td'); + expect(firstCell?.textContent).toBe(evil); + expect(firstCell?.children.length).toBe(0); + + // Exactly one edit button exists (no extra nodes injected) and it carries + // the value as an inert data-* attribute rather than as markup. + const btns = container.querySelectorAll('button.ladder-edit-btn'); + expect(btns.length).toBe(1); + expect(btns[0]!.dataset['provider']).toBe(evil); + }); + }); + + describe('saveLadderConfig max_hourly guard', () => { + // Fill the modal form with a valid baseline so save reaches the store, + // then override max-hourly per case. + async function primeValidForm(maxHourly: string): Promise { + await renderSection(); + (document.getElementById('ladder-cfg-account') as HTMLInputElement).value = 'acct-1'; + (document.getElementById('ladder-cfg-ramp-schedule') as HTMLTextAreaElement).value = + '{"steps":[{"after_days":0,"fraction":1.0}]}'; + (document.getElementById('ladder-cfg-max-hourly') as HTMLInputElement).value = maxHourly; + } + + test('blank max-hourly serializes to null in the payload', async () => { + await primeValidForm(''); + (api.upsertLadderConfig as jest.Mock).mockResolvedValue(baseConfig()); + + await saveLadderConfig(); + + expect(api.upsertLadderConfig).toHaveBeenCalledTimes(1); + const sent = (api.upsertLadderConfig as jest.Mock).mock.calls[0][0] as LadderConfig; + expect(sent.max_hourly_commit_per_run).toBeNull(); + }); + + test('positive max-hourly passes through unchanged', async () => { + await primeValidForm('5'); + (api.upsertLadderConfig as jest.Mock).mockResolvedValue(baseConfig()); + + await saveLadderConfig(); + + expect(api.upsertLadderConfig).toHaveBeenCalledTimes(1); + const sent = (api.upsertLadderConfig as jest.Mock).mock.calls[0][0] as LadderConfig; + expect(sent.max_hourly_commit_per_run).toBe(5); + }); + + test('non-positive max-hourly is rejected (guarded, no store call)', async () => { + await primeValidForm('-1'); + await saveLadderConfig(); + + expect(api.upsertLadderConfig).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith( + expect.objectContaining({ kind: 'error' }), + ); + }); + }); +}); diff --git a/frontend/src/api/index.ts b/frontend/src/api/index.ts index c7c5e93d7..e3fb66f73 100644 --- a/frontend/src/api/index.ts +++ b/frontend/src/api/index.ts @@ -200,6 +200,10 @@ export { approveRIExchange } from './riexchange'; +// Re-export commitment-laddering functions and types (issue #1336) +export type { LadderConfig, LadderRampStep } from './ladder'; +export { getLadderConfigs, upsertLadderConfig } from './ladder'; + // Re-export registrations functions and types export type { AccountRegistration } from './registrations'; export { diff --git a/frontend/src/api/ladder.ts b/frontend/src/api/ladder.ts new file mode 100644 index 000000000..53beee477 --- /dev/null +++ b/frontend/src/api/ladder.ts @@ -0,0 +1,74 @@ +/** + * Commitment Laddering API functions (issue #1333 phase 3). + * + * The feature is flag-gated default-off: the global kill-switch + * (global_config.laddering_enabled) must be true AND the per-account + * LadderConfig.enabled must be true before any laddering engine run fires. + */ + +import { apiRequest } from './client'; + +/** + * A single ramp step within a ladder ramp schedule. + * AfterDays is the delay from run start; Fraction is the share of the + * total target allocated by this tranche (fractions must sum to 1.0). + */ +export interface LadderRampStep { + after_days: number; + fraction: number; +} + +/** + * Per-account, per-provider ladder configuration. + * + * Mode controls whether runs require human approval before executing: + * email_approval - sends an approval email; purchases fire only after approval + * auto_approve - purchases fire immediately (no human gate) + * + * Cadence controls how often the engine runs: + * daily - once per day + * weekly - once per week + * + * All numeric money fields use number|null rather than 0 so absent/unconfigured + * values are distinguishable from a deliberately configured $0. + */ +export interface LadderConfig { + id?: string; + cloud_account_id: string; + provider: string; + enabled: boolean; + mode: 'email_approval' | 'auto_approve'; + cadence: 'daily' | 'weekly'; + target_coverage: number; + buffer_fraction: number; + baseline_percentile: number; + lookback_days: number; + buffer_utilization_threshold: number; + /** null = no cap on hourly commitment delta per run */ + max_hourly_commit_per_run: number | null; + max_actions_per_run: number; + ramp_schedule: { steps: LadderRampStep[] }; + created_at?: string; + updated_at?: string; +} + +/** + * List all per-account ladder configurations. + * Requires view:config permission. + */ +export async function getLadderConfigs(): Promise { + const resp = await apiRequest<{ configs: LadderConfig[] }>('/ladder/configs'); + return resp.configs ?? []; +} + +/** + * Upsert (insert or update) a per-account ladder configuration. + * The upsert key is (cloud_account_id, provider). + * Requires update:config permission. + */ +export async function upsertLadderConfig(cfg: LadderConfig): Promise { + return apiRequest('/ladder/configs', { + method: 'PUT', + body: JSON.stringify(cfg), + }); +} diff --git a/frontend/src/api/settings.ts b/frontend/src/api/settings.ts index b9e4f42bb..1ba457400 100644 --- a/frontend/src/api/settings.ts +++ b/frontend/src/api/settings.ts @@ -24,9 +24,14 @@ export async function updateServiceConfig(provider: string, service: string, cfg } /** - * Update global configuration + * Update global configuration. + * + * Accepts a partial config: the backend merges the body over the stored config + * (any key absent from the JSON keeps its persisted value), so callers may send + * only the fields they intend to change (e.g. the laddering kill-switch toggle + * sends just { laddering_enabled }). A full Config is still valid input. */ -export async function updateConfig(config: Config): Promise { +export async function updateConfig(config: Partial): Promise { return apiRequest('/config', { method: 'PUT', body: JSON.stringify(config) diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 61168840f..afd50df27 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -304,6 +304,10 @@ export interface Config { // Must be 7, 30, or 60 (AWS LookbackPeriodInDays enum). Default: 7. // GCP CUD Recommender has no equivalent parameter; applies to AWS only. recommendations_lookback_days?: number; + // Global kill-switch for the commitment-laddering feature (issue #1336). + // Default false. When true, per-account LadderConfig.enabled settings + // determine whether the engine runs for that account. + laddering_enabled?: boolean; } export interface ServiceConfig { diff --git a/frontend/src/index.html b/frontend/src/index.html index ae9280bb4..aab91e8e6 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -630,6 +630,9 @@
Cloud Storage
+ +
+
diff --git a/frontend/src/ladder.ts b/frontend/src/ladder.ts new file mode 100644 index 000000000..833a3dce0 --- /dev/null +++ b/frontend/src/ladder.ts @@ -0,0 +1,489 @@ +/** + * Commitment Laddering settings section (issue #1333 phase 3). + * + * Renders the per-account ladder config editor inside the + * #commitment-laddering-settings placeholder div that lives inside the + * Purchasing Settings panel. The section is flag-gated default-off: + * + * - Global kill-switch: global_config.laddering_enabled (bool) + * - Per-account: LadderConfig.enabled (bool) per (account, provider) pair + * + * No laddering engine runs fire until both flags are true. This PR (phase 3, + * schema foundation) wires the UI to read and write configs; actual engine + * invocation is in a later phase. + */ + +import * as api from './api'; +import { escapeHtml, formatDate } from './utils'; +import { showToast } from './toast'; +import { canAccess } from './permissions'; + +// ========================================== +// MODULE STATE +// ========================================== + +// Separate in-flight guards for the two independent save paths. Sharing one +// flag let the kill-switch toggle and the per-account modal save block/revert +// each other even though they touch unrelated backend endpoints. +let killSwitchSaveInFlight = false; +let configModalSaveInFlight = false; + +// Cached configs from the last successful load. +let cachedConfigs: api.LadderConfig[] = []; + +// ========================================== +// PUBLIC API +// ========================================== + +/** + * Initialize the Commitment Laddering settings section. + * + * Renders the HTML form into #commitment-laddering-settings and populates + * it with data from the API. Should be called from loadGlobalSettings after + * the global config response is available. + * + * The globalEnabled parameter is the current value of + * global_config.laddering_enabled so the kill-switch toggle reflects the + * persisted state without a second API call. + */ +export async function initLadderingSettings(globalEnabled: boolean): Promise { + const container = document.getElementById('commitment-laddering-settings'); + if (!container) return; + + container.innerHTML = renderLadderingSection(globalEnabled); + wireKillSwitchToggle(); + wireModalCloseButtons(); + + try { + cachedConfigs = await api.getLadderConfigs(); + renderConfigTable(cachedConfigs); + } catch (err) { + console.error('Failed to load ladder configs:', err); + const tableContainer = document.getElementById('ladder-configs-table-container'); + if (tableContainer) { + tableContainer.innerHTML = '

Failed to load per-account configurations.

'; + } + } + + if (canAccess('update', 'config')) { + document.getElementById('ladder-add-config-btn')?.addEventListener('click', () => openLadderConfigModal()); + } +} + +// ========================================== +// RENDERING +// ========================================== + +function renderLadderingSection(globalEnabled: boolean): string { + const canEdit = canAccess('update', 'config'); + const disabledAttr = canEdit ? '' : ' disabled'; + + return ` +
+ Commitment Laddering +

+ Commitment Laddering automatically ramps up cloud commitments towards a + target coverage level using a configurable ramp schedule. It is feature-gated: + enable the global kill-switch below, then enable individual accounts via the + per-account configuration table. +

+ +
+
+ + Global kill-switch. Must be on before any per-account config can fire. +
+
+ +
+
+ +
+

Per-Account Configurations

+
+

Loading…

+
+ ${canEdit ? ` + ` : ''} +
+
+ + + +`; +} + +// Exported for unit testing (mirrors the apikeys.ts convention of exposing +// render/action helpers so tests can drive them directly). +export function renderConfigTable(configs: api.LadderConfig[]): void { + const container = document.getElementById('ladder-configs-table-container'); + if (!container) return; + + if (configs.length === 0) { + container.innerHTML = '

No per-account configurations yet. Click Add Account Config to create one.

'; + return; + } + + const canEdit = canAccess('update', 'config'); + const rows = configs.map(cfg => ` + + ${escapeHtml(cfg.cloud_account_id)} + ${escapeHtml(cfg.provider.toUpperCase())} + ${cfg.enabled ? 'Yes' : 'No'} + ${escapeHtml(cfg.mode === 'email_approval' ? 'Email Approval' : 'Auto Approve')} + ${escapeHtml(cfg.cadence === 'daily' ? 'Daily' : 'Weekly')} + ${cfg.target_coverage.toFixed(1)}% + ${cfg.updated_at ? escapeHtml(formatDate(cfg.updated_at)) : 'N/A'} + ${canEdit + ? `` + : ''} + + `).join(''); + + container.innerHTML = ` + + + + + + + + + + + + + + ${rows} +
Account IDProviderEnabledModeCadenceTarget CoverageUpdated
+ `; + + // Wire edit buttons via querySelectorAll so no user data is ever injected + // into a JS string context (data-* attributes are HTML-attribute-context only). + container.querySelectorAll('.ladder-edit-btn').forEach(btn => { + btn.addEventListener('click', () => { + const accountID = btn.dataset['accountId'] ?? ''; + const provider = btn.dataset['provider'] ?? ''; + const cfg = cachedConfigs.find(c => c.cloud_account_id === accountID && c.provider === provider); + if (cfg) openLadderConfigModal(cfg); + }); + }); +} + +// ========================================== +// MODAL CLOSE WIRING +// ========================================== + +// closeLadderModal hides the per-account config modal. +function closeLadderModal(): void { + document.getElementById('ladder-config-modal')?.classList.add('hidden'); +} + +// wireModalCloseButtons attaches click listeners to the modal's × and Cancel +// buttons. Uses addEventListener rather than inline onclick for CSP +// consistency with the dynamically-wired edit buttons (no inline handlers +// anywhere in this module). +function wireModalCloseButtons(): void { + document.getElementById('ladder-modal-close-btn')?.addEventListener('click', closeLadderModal); + document.getElementById('ladder-modal-cancel-btn')?.addEventListener('click', closeLadderModal); +} + +// ========================================== +// KILL-SWITCH TOGGLE +// ========================================== + +function wireKillSwitchToggle(): void { + const toggle = document.getElementById('setting-laddering-enabled') as HTMLInputElement | null; + if (!toggle || !canAccess('update', 'config')) return; + + toggle.addEventListener('change', async () => { + if (killSwitchSaveInFlight) { + toggle.checked = !toggle.checked; // revert optimistic + return; + } + killSwitchSaveInFlight = true; + try { + // The backend merges a partial PUT over the stored config (json.Unmarshal + // only sets keys present in the body), so we send ONLY the field we are + // changing. Every other setting is preserved server-side; enumerating the + // whole config here was fragile and dropped fields it did not list. + await api.updateConfig({ laddering_enabled: toggle.checked }); + showToast({ + message: `Commitment Laddering ${toggle.checked ? 'enabled' : 'disabled'} globally.`, + kind: 'success', + }); + } catch (err) { + toggle.checked = !toggle.checked; // revert on error + showToast({ message: 'Failed to update laddering kill-switch.', kind: 'error' }); + console.error('Failed to toggle laddering_enabled:', err); + } finally { + killSwitchSaveInFlight = false; + } + }); +} + +// ========================================== +// PER-ACCOUNT CONFIG MODAL +// ========================================== + +function openLadderConfigModal(existing?: api.LadderConfig): void { + const modal = document.getElementById('ladder-config-modal'); + if (!modal) return; + + // Populate modal fields. + setValue('ladder-cfg-id', existing?.id ?? ''); + setValue('ladder-cfg-account', existing?.cloud_account_id ?? ''); + setSelectValue('ladder-cfg-provider', existing?.provider ?? 'aws'); + setChecked('ladder-cfg-enabled', existing?.enabled ?? false); + setSelectValue('ladder-cfg-mode', existing?.mode ?? 'email_approval'); + setSelectValue('ladder-cfg-cadence', existing?.cadence ?? 'daily'); + setValue('ladder-cfg-target-coverage', String(existing?.target_coverage ?? 100)); + setValue('ladder-cfg-buffer-fraction', String(existing?.buffer_fraction ?? 0.10)); + setValue('ladder-cfg-baseline-percentile', String(existing?.baseline_percentile ?? 5.0)); + setValue('ladder-cfg-lookback-days', String(existing?.lookback_days ?? 30)); + setValue('ladder-cfg-buf-util-threshold', String(existing?.buffer_utilization_threshold ?? 90)); + setValue('ladder-cfg-max-hourly', existing?.max_hourly_commit_per_run != null + ? String(existing.max_hourly_commit_per_run) + : ''); + setValue('ladder-cfg-max-actions', String(existing?.max_actions_per_run ?? 10)); + + const defaultRamp = JSON.stringify({ steps: [{ after_days: 0, fraction: 1.0 }] }, null, 2); + setValue('ladder-cfg-ramp-schedule', + existing?.ramp_schedule ? JSON.stringify(existing.ramp_schedule, null, 2) : defaultRamp); + + // Disable account/provider fields when editing an existing row (they form + // the upsert key and cannot be changed without a delete+re-create). + const accountInput = document.getElementById('ladder-cfg-account') as HTMLInputElement | null; + const providerSelect = document.getElementById('ladder-cfg-provider') as HTMLSelectElement | null; + if (accountInput) accountInput.readOnly = !!existing; + if (providerSelect) providerSelect.disabled = !!existing; + + // Wire the save button (remove previous listener by replacing the element + // clone so duplicate-listener accumulation cannot occur). + const form = document.getElementById('ladder-config-form'); + if (form) { + const newForm = form.cloneNode(true) as HTMLElement; + form.replaceWith(newForm); + newForm.addEventListener('submit', (e) => { + e.preventDefault(); + saveLadderConfig(); + }); + // The Cancel button lives inside the form, so cloneNode drops its + // listener (cloneNode does not copy event handlers). Re-wire it on the + // fresh node. The × close button sits outside the form and keeps the + // listener attached once in wireModalCloseButtons. + newForm.querySelector('#ladder-modal-cancel-btn') + ?.addEventListener('click', closeLadderModal); + } + + modal.classList.remove('hidden'); +} + +// Exported for unit testing (see renderConfigTable note). +export async function saveLadderConfig(): Promise { + if (configModalSaveInFlight) return; + + const maxHourlyRaw = (document.getElementById('ladder-cfg-max-hourly') as HTMLInputElement | null)?.value?.trim() ?? ''; + const maxHourly: number | null = maxHourlyRaw === '' ? null : Number(maxHourlyRaw); + if (maxHourlyRaw !== '' && (!Number.isFinite(maxHourly) || (maxHourly as number) <= 0)) { + showToast({ message: 'Max hourly commit must be a positive number (or blank for no cap).', kind: 'error' }); + return; + } + + const rampRaw = (document.getElementById('ladder-cfg-ramp-schedule') as HTMLTextAreaElement | null)?.value?.trim() ?? ''; + let rampSchedule: api.LadderConfig['ramp_schedule']; + try { + rampSchedule = JSON.parse(rampRaw); + } catch { + showToast({ message: 'Ramp schedule is not valid JSON.', kind: 'error' }); + return; + } + + const cfg: api.LadderConfig = { + id: (document.getElementById('ladder-cfg-id') as HTMLInputElement | null)?.value || undefined, + cloud_account_id: (document.getElementById('ladder-cfg-account') as HTMLInputElement | null)?.value?.trim() ?? '', + provider: (document.getElementById('ladder-cfg-provider') as HTMLSelectElement | null)?.value ?? 'aws', + enabled: (document.getElementById('ladder-cfg-enabled') as HTMLInputElement | null)?.checked ?? false, + mode: ((document.getElementById('ladder-cfg-mode') as HTMLSelectElement | null)?.value ?? 'email_approval') as api.LadderConfig['mode'], + cadence: ((document.getElementById('ladder-cfg-cadence') as HTMLSelectElement | null)?.value ?? 'daily') as api.LadderConfig['cadence'], + target_coverage: Number((document.getElementById('ladder-cfg-target-coverage') as HTMLInputElement | null)?.value ?? '100'), + buffer_fraction: Number((document.getElementById('ladder-cfg-buffer-fraction') as HTMLInputElement | null)?.value ?? '0.10'), + baseline_percentile: Number((document.getElementById('ladder-cfg-baseline-percentile') as HTMLInputElement | null)?.value ?? '5'), + lookback_days: Number((document.getElementById('ladder-cfg-lookback-days') as HTMLInputElement | null)?.value ?? '30'), + buffer_utilization_threshold: Number((document.getElementById('ladder-cfg-buf-util-threshold') as HTMLInputElement | null)?.value ?? '90'), + max_hourly_commit_per_run: maxHourly, + max_actions_per_run: Number((document.getElementById('ladder-cfg-max-actions') as HTMLInputElement | null)?.value ?? '10'), + ramp_schedule: rampSchedule, + }; + + if (!cfg.cloud_account_id) { + showToast({ message: 'Cloud Account ID is required.', kind: 'error' }); + return; + } + + configModalSaveInFlight = true; + const saveBtn = document.getElementById('ladder-config-save-btn') as HTMLButtonElement | null; + if (saveBtn) saveBtn.disabled = true; + + try { + const saved = await api.upsertLadderConfig(cfg); + + // Update the in-memory cache and re-render the table. + const idx = cachedConfigs.findIndex( + c => c.cloud_account_id === saved.cloud_account_id && c.provider === saved.provider + ); + if (idx >= 0) { + cachedConfigs[idx] = saved; + } else { + cachedConfigs.push(saved); + } + renderConfigTable(cachedConfigs); + + closeLadderModal(); + showToast({ message: 'Ladder config saved.', kind: 'success' }); + } catch (err) { + showToast({ message: 'Failed to save ladder config. Check inputs and try again.', kind: 'error' }); + console.error('saveLadderConfig:', err); + } finally { + configModalSaveInFlight = false; + if (saveBtn) saveBtn.disabled = false; + } +} + +// ========================================== +// DOM HELPERS +// ========================================== + +function setValue(id: string, value: string): void { + const el = document.getElementById(id) as HTMLInputElement | HTMLTextAreaElement | null; + if (el) el.value = value; +} + +function setSelectValue(id: string, value: string): void { + const el = document.getElementById(id) as HTMLSelectElement | null; + if (el) el.value = value; +} + +function setChecked(id: string, checked: boolean): void { + const el = document.getElementById(id) as HTMLInputElement | null; + if (el) el.checked = checked; +} diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 249cbe0e3..8c1376f2b 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -19,6 +19,7 @@ import { readServiceFilterControls, isServiceFilterError, } from './serviceFilters'; +import { initLadderingSettings } from './ladder'; /** * Issue #196 — after any mutation to an `account_service_overrides` row @@ -3165,6 +3166,11 @@ export async function loadGlobalSettings(): Promise { // Update visibility based on loaded settings updateProviderSettingsVisibility(); updateCollectionScheduleVisibility(); + + // Render the Commitment Laddering section (flag-gated default-off, + // issue #1333 phase 3). Runs asynchronously; failures are surfaced + // inside the section without blocking the rest of Settings. + void initLadderingSettings(data.global.laddering_enabled ?? false); } const services = data.services ?? []; @@ -3435,6 +3441,16 @@ export async function saveGlobalSettings(e: Event): Promise { recommendations_lookback_days: parseInt(byId('setting-recs-lookback-days')?.value || '7', 10), }; + // Include laddering_enabled in the payload when the Purchasing panel's + // toggle is present in the DOM (i.e. initLadderingSettings has run). + // When absent (General panel), the backend's updateConfig merges this PUT + // over the stored config, so an omitted laddering_enabled keeps its + // persisted value rather than being reset. + const ladderingToggle = byId('setting-laddering-enabled'); + if (ladderingToggle !== null) { + settings.laddering_enabled = ladderingToggle.checked; + } + try { await api.updateConfig(settings); diff --git a/frontend/src/types.ts b/frontend/src/types.ts index c04d39308..add694e1f 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -367,6 +367,11 @@ export interface GlobalConfig { recommendations_cache_stale_hours?: number; // AWS Cost Explorer lookback window (days). One of 7, 30, or 60. Default: 7. recommendations_lookback_days?: number; + // Global kill-switch for the commitment-laddering feature (issue #1333 phase 3). + // When false (the default) no laddering engine runs fire, regardless of + // per-account LadderConfig settings. Set to true to allow per-account + // configs to activate individually. + laddering_enabled?: boolean; } // API Keys types diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 5274c1548..05ae41645 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -737,3 +737,20 @@ func (m *mockConfigStore) GetPendingExecutionsTx(ctx context.Context, _ pgx.Tx) return m.GetPendingExecutions(ctx) } func (m *mockConfigStore) WithTx(_ context.Context, fn func(tx pgx.Tx) error) error { return fn(nil) } + +func (m *mockConfigStore) GetLadderConfigs(_ context.Context) ([]config.LadderConfigDB, error) { + return nil, nil +} +func (m *mockConfigStore) GetLadderConfig(_ context.Context, _, _ string) (*config.LadderConfigDB, error) { + return nil, nil +} +func (m *mockConfigStore) UpsertLadderConfig(_ context.Context, cfg *config.LadderConfigDB) (*config.LadderConfigDB, error) { + return cfg, nil +} +func (m *mockConfigStore) UpdateGlobalConfigAtomic(_ context.Context, apply func(*config.GlobalConfig) error) (*config.GlobalConfig, error) { + cfg := &config.GlobalConfig{} + if err := apply(cfg); err != nil { + return nil, err + } + return cfg, nil +} diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index ad0e72a73..d385f8e3e 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -255,7 +255,9 @@ func TestHandler_sendPurchaseApprovalEmail_ConfigError(t *testing.T) { } // --------------------------------------------------------------------------- -// updateRIExchangeConfig — GetGlobalConfig error after validation passes +// updateRIExchangeConfig — store error after validation passes. The read now +// happens inside the serialized UpdateGlobalConfigAtomic call, so a store-read +// failure surfaces through that unified path (still a 500-class error). // --------------------------------------------------------------------------- func TestHandler_updateRIExchangeConfig_GetGlobalConfigError(t *testing.T) { @@ -274,7 +276,7 @@ func TestHandler_updateRIExchangeConfig_GetGlobalConfigError(t *testing.T) { } _, err := h.updateRIExchangeConfig(ctx, req) assert.Error(t, err) - assert.Contains(t, err.Error(), "failed to load config") + assert.Contains(t, err.Error(), "failed to save config") } func TestHandler_updateRIExchangeConfig_SaveError(t *testing.T) { diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index 615373d5d..9daa5576f 100644 --- a/internal/api/handler_config.go +++ b/internal/api/handler_config.go @@ -58,85 +58,101 @@ func (h *Handler) getConfig(ctx context.Context, req *events.LambdaFunctionURLRe return resp, nil } -// preserveOmittedRecommendationFields merges persisted GlobalConfig values -// for the cycle-parameter fields when the request body omits them. -// Without this merge, a partial PUT would silently zero out -// RecommendationsCacheStaleHours / RecommendationsLookbackDays / PurchaseDelayHours, -// which all have meaningful 0-vs-omitted semantics that json.Unmarshal can't -// represent directly. Errors from GetGlobalConfig fall through: the request body's -// zero values then flow into Validate() which rejects out-of-range values, -// matching the pre-fix behaviour. Extracted from updateConfig to keep that -// function under the cyclomatic-complexity gate after the merge logic was -// added (PR #308 CodeRabbit pass-2 review). -func (h *Handler) preserveOmittedRecommendationFields(ctx context.Context, cfg *config.GlobalConfig, body string) error { - var present map[string]json.RawMessage - if err := json.Unmarshal([]byte(body), &present); err != nil { - return NewClientError(400, "invalid request body") - } - _, hasStale := present["recommendations_cache_stale_hours"] - _, hasLookback := present["recommendations_lookback_days"] - _, hasDelay := present["purchase_delay_hours"] - if hasStale && hasLookback && hasDelay { - return nil - } - existing, gcErr := h.config.GetGlobalConfig(ctx) - if gcErr != nil || existing == nil { - return nil - } - if !hasStale { - cfg.RecommendationsCacheStaleHours = existing.RecommendationsCacheStaleHours - } - if !hasLookback { - cfg.RecommendationsLookbackDays = existing.RecommendationsLookbackDays - } - if !hasDelay { - cfg.PurchaseDelayHours = existing.PurchaseDelayHours - } - return nil -} - func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionURLRequest) (*StatusResponse, error) { // Require update:config permission if _, err := h.requirePermission(ctx, req, "update", "config"); err != nil { return nil, err } - var cfg config.GlobalConfig - if err := json.Unmarshal([]byte(req.Body), &cfg); err != nil { + // Reject a malformed body before any DB work so a bad request fails fast + // (and never opens a transaction). Keeps the nil-store fast-fail contract. + if !json.Valid([]byte(req.Body)) { return nil, NewClientError(400, "invalid request body") } - if err := h.preserveOmittedRecommendationFields(ctx, &cfg, req.Body); err != nil { + // Presence map of the top-level keys the caller actually sent. Used to + // (a) replace (not merge) grace_period_days when present, and (b) gate the + // service-config propagation on the global defaults actually being sent, so + // a deliberately-partial PUT (e.g. the kill-switch toggle) does not rewrite + // per-service customizations. + var present map[string]json.RawMessage + if err := json.Unmarshal([]byte(req.Body), &present); err != nil { + return nil, NewClientError(400, "invalid request body") + } + _, gracePresent := present["grace_period_days"] + + // Serialized read-modify-write: the store loads the stored config and + // applies this closure under an advisory-locked transaction, then upserts + // the result in the same tx. json.Unmarshal only assigns keys present in the + // body, so any field ABSENT is preserved (present -> updated, absent -> + // preserved) for EVERY field, with no per-field enumeration. Doing the read + // and write in one locked tx prevents two concurrent partial PUTs (e.g. the + // laddering kill-switch toggle and a Settings save) from reading the same + // stale base and losing each other's update. The closure returns a + // ClientError(400) on a bad body or validation failure, which the store + // propagates unchanged; DB/transport errors surface as 500. + cfg, err := h.config.UpdateGlobalConfigAtomic(ctx, func(existing *config.GlobalConfig) error { + // grace_period_days is a map: json.Unmarshal into a non-nil map MERGES + // keys (an omitted key can never be deleted). When the caller sends the + // key, nil the stored map first so the body's map REPLACES it wholesale + // (present -> replace); when absent, leave it to preserve. + if gracePresent { + existing.GracePeriodDays = nil + } + if uErr := json.Unmarshal([]byte(req.Body), existing); uErr != nil { + return NewClientError(400, "invalid request body") + } + if vErr := existing.Validate(); vErr != nil { + return NewClientError(400, fmt.Sprintf("validation error: %s", vErr)) + } + return nil + }) + if err != nil { return nil, err } - // Validate the configuration - if err := cfg.Validate(); err != nil { - return nil, NewClientError(400, fmt.Sprintf("validation error: %s", err)) + // Propagate global defaults to every service config ONLY when the caller + // actually sent at least one of those defaults. A partial PUT that omits + // them (the kill-switch toggle sends only { laddering_enabled }) must not + // overwrite money-affecting per-service term/payment/coverage/ramp overrides. + if anyKeyPresent(present, "default_term", "default_payment", "default_coverage", "default_ramp_schedule") { + h.propagateGlobalDefaults(ctx, cfg) } - if err := h.config.SaveGlobalConfig(ctx, &cfg); err != nil { - return nil, err + return &StatusResponse{Status: "updated"}, nil +} + +// anyKeyPresent reports whether any of keys is present in m. +func anyKeyPresent(m map[string]json.RawMessage, keys ...string) bool { + for _, k := range keys { + if _, ok := m[k]; ok { + return true + } } + return false +} - // Propagate global defaults to all service configurations +// propagateGlobalDefaults overlays the global term/payment/coverage/ramp +// defaults onto every service config. Best-effort: failures are logged, not +// returned, because the global config was already saved. +func (h *Handler) propagateGlobalDefaults(ctx context.Context, cfg *config.GlobalConfig) { services, err := h.config.ListServiceConfigs(ctx) if err != nil { - // Log but don't fail - global config was saved logging.Warnf("Failed to list service configs for propagation: %v", err) - } else { - for _, svc := range services { - svc.Term = cfg.DefaultTerm - svc.Payment = cfg.DefaultPayment - svc.Coverage = cfg.DefaultCoverage - svc.RampSchedule = cfg.DefaultRampSchedule - if err := h.config.SaveServiceConfig(ctx, &svc); err != nil { - logging.Warnf("Failed to update service config %s/%s: %v", svc.Provider, svc.Service, err) - } + return + } + // Index-based iteration mutates each element in place and avoids copying the + // (large) ServiceConfig struct per iteration (gocritic rangeValCopy). + for i := range services { + svc := &services[i] + svc.Term = cfg.DefaultTerm + svc.Payment = cfg.DefaultPayment + svc.Coverage = cfg.DefaultCoverage + svc.RampSchedule = cfg.DefaultRampSchedule + if saveErr := h.config.SaveServiceConfig(ctx, svc); saveErr != nil { + logging.Warnf("Failed to update service config %s/%s: %v", svc.Provider, svc.Service, saveErr) } } - - return &StatusResponse{Status: "updated"}, nil } // checkCommitmentOptionCombo rejects saves that carry a (term, payment) diff --git a/internal/api/handler_config_test.go b/internal/api/handler_config_test.go index 547ed1f21..dedb63c4f 100644 --- a/internal/api/handler_config_test.go +++ b/internal/api/handler_config_test.go @@ -2,6 +2,8 @@ package api import ( "context" + "fmt" + "sync" "testing" "github.com/LeanerCloud/CUDly/internal/config" @@ -87,6 +89,379 @@ func TestHandler_updateConfig(t *testing.T) { assert.Equal(t, "updated", result.Status) } +// TestHandler_updateConfig_LadderingEnabledPreservation asserts the +// kill-switch preservation contract added in the ladder-schema PR: +// - a PUT that OMITS laddering_enabled must NOT reset the persisted value +// (the General-settings panel never sends the field, so a plain save must +// leave a previously-enabled kill-switch on). +// - a PUT that sends laddering_enabled:false must flip it off. +// +// Both cases capture the *config.GlobalConfig handed to SaveGlobalConfig and +// assert on the final LadderingEnabled value the store would persist. +func TestHandler_updateConfig_LadderingEnabledPreservation(t *testing.T) { + // newHandler builds a handler whose GetGlobalConfig returns a config seeded + // with existingLaddering, and captures the config handed to SaveGlobalConfig + // into the returned pointer so callers can assert the persisted value. + newHandler := func(t *testing.T, existingLaddering bool) (*Handler, *config.GlobalConfig) { + t.Helper() + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) }) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + } + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + + // The body always omits the recommendations fields, so updateConfig + // fetches the existing config to preserve them; seed LadderingEnabled + // there so the preservation branch has a concrete value to keep. + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + RecommendationsCacheStaleHours: config.DefaultRecommendationsCacheStaleHours, + RecommendationsLookbackDays: config.DefaultRecommendationsLookbackDays, + LadderingEnabled: existingLaddering, + }, nil) + mockStore.On("ListServiceConfigs", ctx).Return([]config.ServiceConfig{}, nil) + + var saved config.GlobalConfig + mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")). + Run(func(args mock.Arguments) { + saved = *args.Get(1).(*config.GlobalConfig) + }).Return(nil) + + return &Handler{config: mockStore, auth: mockAuth}, &saved + } + + makeReq := func(body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: body, + } + } + + const bodyOmitted = `{"enabled_providers": ["aws"], "default_term": 3}` + bodyWith := func(v bool) string { + return fmt.Sprintf(`{"enabled_providers": ["aws"], "default_term": 3, "laddering_enabled": %t}`, v) + } + + cases := []struct { + name string + body string + existing bool + wantPersisted bool + }{ + {"omitted preserves existing true", bodyOmitted, true, true}, + {"omitted preserves existing false", bodyOmitted, false, false}, + {"explicit false flips existing true off", bodyWith(false), true, false}, + {"explicit true flips existing false on", bodyWith(true), false, true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + handler, saved := newHandler(t, tc.existing) + + result, err := handler.updateConfig(ctx, makeReq(tc.body)) + require.NoError(t, err) + assert.Equal(t, "updated", result.Status) + assert.Equal(t, tc.wantPersisted, saved.LadderingEnabled, + "persisted laddering_enabled mismatch for case %q", tc.name) + }) + } +} + +// TestHandler_updateConfig_PartialPUTPreservesOmittedFields is the F2 +// data-loss regression test. The laddering kill-switch toggle sends a partial +// PUT of only {"laddering_enabled": true}; before the merge fix, updateConfig +// unmarshalled that into a zero GlobalConfig and only restored the four +// recommendation/laddering fields, silently zeroing approval_required, +// default_ramp_schedule, and every ri_exchange_* field (wiping the user's +// RI-exchange automation). The merge must leave every omitted field at its +// stored value while applying the one field the body carries. Assert against +// the config handed to SaveGlobalConfig, not just a 200. +func TestHandler_updateConfig_PartialPUTPreservesOmittedFields(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) }) + + mockAuth.On("ValidateSession", ctx, "admin-token"). + Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + mockAuth.grantAdmin() + + notifyEmail := "ops@example.com" + existing := &config.GlobalConfig{ + EnabledProviders: []string{"aws"}, + NotificationEmail: ¬ifyEmail, + AutoCollect: true, + CollectionSchedule: "daily", + NotificationDaysBefore: 3, + ApprovalRequired: true, + DefaultTerm: 3, + DefaultPayment: "all-upfront", + DefaultCoverage: 80, + DefaultRampSchedule: "immediate", + RIExchangeEnabled: true, + RIExchangeMode: "automatic", + RIExchangeUtilizationThreshold: 95, + RIExchangeMaxPerExchangeUSD: 1000, + RIExchangeMaxDailyUSD: 5000, + RIExchangeLookbackDays: 30, + RecommendationsCacheStaleHours: 24, + RecommendationsLookbackDays: 7, + PurchaseDelayHours: 48, + LadderingEnabled: false, + } + mockStore.On("GetGlobalConfig", ctx).Return(existing, nil) + + var saved config.GlobalConfig + mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")). + Run(func(args mock.Arguments) { saved = *args.Get(1).(*config.GlobalConfig) }). + Return(nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"laddering_enabled": true}`, + } + result, err := handler.updateConfig(ctx, req) + require.NoError(t, err) + assert.Equal(t, "updated", result.Status) + + // F3: a PUT that carries none of the global defaults must NOT trigger the + // service-config propagation loop (which would overwrite per-service + // term/payment/coverage/ramp customizations). + mockStore.AssertNotCalled(t, "ListServiceConfigs", mock.Anything) + mockStore.AssertNotCalled(t, "SaveServiceConfig", mock.Anything, mock.Anything) + + // The one field the body carried was applied. + assert.True(t, saved.LadderingEnabled, "laddering_enabled from the body must be applied") + + // Every omitted field must retain its stored value (the F2 bug zeroed these). + assert.True(t, saved.ApprovalRequired, "approval_required must be preserved") + assert.Equal(t, "immediate", saved.DefaultRampSchedule, "default_ramp_schedule must be preserved") + assert.True(t, saved.RIExchangeEnabled, "ri_exchange_enabled must be preserved") + assert.Equal(t, "automatic", saved.RIExchangeMode, "ri_exchange_mode must be preserved") + assert.Equal(t, 95.0, saved.RIExchangeUtilizationThreshold, "ri_exchange_utilization_threshold must be preserved") + assert.Equal(t, 1000.0, saved.RIExchangeMaxPerExchangeUSD, "ri_exchange_max_per_exchange_usd must be preserved") + assert.Equal(t, 5000.0, saved.RIExchangeMaxDailyUSD, "ri_exchange_max_daily_usd must be preserved") + assert.Equal(t, 30, saved.RIExchangeLookbackDays, "ri_exchange_lookback_days must be preserved") + + // Other scalars stay intact too. + assert.Equal(t, []string{"aws"}, saved.EnabledProviders) + assert.Equal(t, "all-upfront", saved.DefaultPayment) + assert.Equal(t, 80.0, saved.DefaultCoverage) + assert.Equal(t, 48, saved.PurchaseDelayHours) + require.NotNil(t, saved.NotificationEmail) + assert.Equal(t, "ops@example.com", *saved.NotificationEmail) +} + +// TestHandler_updateConfig_UsesAtomicPath asserts updateConfig performs its +// read-modify-write through UpdateGlobalConfigAtomic (the serialized, +// advisory-locked store path) rather than a separate Get + Save, which is what +// prevents the concurrent-partial-PUT lost update. The explicit expectation +// means the mock dispatches through Called (it does NOT fall back to the +// Get+Save emulation), so a passing run proves the handler invoked the atomic +// method; the Run callback exercises the exact apply closure the handler builds. +func TestHandler_updateConfig_UsesAtomicPath(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) }) + + mockAuth.On("ValidateSession", ctx, "admin-token"). + Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + mockAuth.grantAdmin() + + // The config the atomic apply runs against (as if freshly read under lock). + stored := &config.GlobalConfig{ + EnabledProviders: []string{"aws"}, + ApprovalRequired: true, + DefaultTerm: 3, + DefaultPayment: "all-upfront", + DefaultCoverage: 80, + CollectionSchedule: "daily", + NotificationDaysBefore: 3, + RIExchangeEnabled: true, + RIExchangeMode: "manual", + RIExchangeLookbackDays: 30, + RecommendationsCacheStaleHours: 24, + RecommendationsLookbackDays: 7, + LadderingEnabled: false, + } + + var merged config.GlobalConfig + mockStore.On("UpdateGlobalConfigAtomic", ctx, mock.AnythingOfType("func(*config.GlobalConfig) error")). + Run(func(args mock.Arguments) { + apply := args.Get(1).(func(*config.GlobalConfig) error) + cfg := *stored + require.NoError(t, apply(&cfg)) + merged = cfg + }). + Return(&merged, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"laddering_enabled": true}`, + } + result, err := handler.updateConfig(ctx, req) + require.NoError(t, err) + assert.Equal(t, "updated", result.Status) + + // laddering-only PUT carries no global defaults, so propagation is skipped. + mockStore.AssertNotCalled(t, "ListServiceConfigs", mock.Anything) + + // The apply closure merged the body over the stored config: the sent field + // is applied and omitted fields are preserved. + assert.True(t, merged.LadderingEnabled, "sent field applied") + assert.True(t, merged.ApprovalRequired, "omitted approval_required preserved") + assert.True(t, merged.RIExchangeEnabled, "omitted ri_exchange_enabled preserved") +} + +// TestHandler_updateConfig_GracePeriodDaysReplaceNotMerge is the F4 regression. +// json.Unmarshal into a pre-populated (non-nil) map MERGES keys, so a PUT that +// carries grace_period_days could never delete a previously-set provider key. +// When the caller sends the key, updateConfig nils the stored map first so the +// body's map REPLACES it wholesale. Sending only { "aws": 10 } must drop the +// prior azure/gcp entries. +func TestHandler_updateConfig_GracePeriodDaysReplaceNotMerge(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) }) + + mockAuth.On("ValidateSession", ctx, "admin-token"). + Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + mockAuth.grantAdmin() + + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + EnabledProviders: []string{"aws"}, + DefaultTerm: 3, + DefaultPayment: "all-upfront", + DefaultCoverage: 80, + CollectionSchedule: "daily", + NotificationDaysBefore: 3, + RecommendationsCacheStaleHours: 24, + RecommendationsLookbackDays: 7, + GracePeriodDays: map[string]int{"aws": 7, "azure": 14, "gcp": 3}, + }, nil) + + var saved config.GlobalConfig + mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")). + Run(func(args mock.Arguments) { saved = *args.Get(1).(*config.GlobalConfig) }). + Return(nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"grace_period_days": {"aws": 10}}`, + } + _, err := handler.updateConfig(ctx, req) + require.NoError(t, err) + + assert.Equal(t, map[string]int{"aws": 10}, saved.GracePeriodDays, + "grace_period_days must be replaced wholesale, not merged (azure/gcp dropped)") + // grace_period_days is not a propagation default, so propagation is skipped. + mockStore.AssertNotCalled(t, "ListServiceConfigs", mock.Anything) +} + +// serializedConfigStore is a concurrency-test store double. Its +// UpdateGlobalConfigAtomic guards the read-modify-write with a real mutex, +// modeling the transaction-scoped advisory lock that +// PostgresStore.UpdateGlobalConfigAtomic takes. It embeds *MockConfigStore only +// to satisfy the rest of StoreInterface; updateConfig calls just the two +// methods overridden here. +type serializedConfigStore struct { + *MockConfigStore + stored *config.GlobalConfig + mu sync.Mutex +} + +func (s *serializedConfigStore) UpdateGlobalConfigAtomic(_ context.Context, apply func(*config.GlobalConfig) error) (*config.GlobalConfig, error) { + s.mu.Lock() + defer s.mu.Unlock() + cfg := *s.stored // read the current committed state under the lock + if err := apply(&cfg); err != nil { + return nil, err + } + s.stored = &cfg // commit before releasing the lock + return &cfg, nil +} + +func (s *serializedConfigStore) ListServiceConfigs(_ context.Context) ([]config.ServiceConfig, error) { + return []config.ServiceConfig{}, nil +} + +// TestHandler_updateConfig_ConcurrentPartialPUTsNoLostUpdate is the F2 +// concurrency regression test (run under -race). Two overlapping partial PUTs +// touch DIFFERENT fields; with the serialized read-modify-write (the store's +// mutex models the advisory lock PostgresStore.UpdateGlobalConfigAtomic takes) +// BOTH updates must survive. A non-atomic Get + merge + Save (the pre-fix path) +// could read the same stale base in both goroutines and let the later save drop +// the earlier change. Coordination uses a WaitGroup + channel (no sleeps); only +// the test goroutine calls require. +func TestHandler_updateConfig_ConcurrentPartialPUTsNoLostUpdate(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "admin-token"). + Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + mockAuth.grantAdmin() + + store := &serializedConfigStore{ + MockConfigStore: new(MockConfigStore), + stored: &config.GlobalConfig{ + EnabledProviders: []string{"aws"}, + ApprovalRequired: true, + DefaultTerm: 3, + DefaultPayment: "all-upfront", + DefaultCoverage: 80, + CollectionSchedule: "daily", + NotificationDaysBefore: 3, + RIExchangeEnabled: true, + RIExchangeMode: "manual", + RIExchangeLookbackDays: 30, + RecommendationsCacheStaleHours: 24, + RecommendationsLookbackDays: 7, + }, + } + handler := &Handler{config: store, auth: mockAuth} + + makeReq := func(body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: body, + } + } + + var wg sync.WaitGroup + errCh := make(chan error, 2) + for _, body := range []string{ + `{"approval_required": false}`, + `{"ri_exchange_max_daily_usd": 12345}`, + } { + wg.Add(1) + go func(b string) { + defer wg.Done() + _, err := handler.updateConfig(ctx, makeReq(b)) + errCh <- err + }(body) + } + wg.Wait() + close(errCh) + for err := range errCh { + require.NoError(t, err) + } + + // Both concurrent partial updates survived (no lost update). + assert.False(t, store.stored.ApprovalRequired, "approval_required update survived") + assert.Equal(t, 12345.0, store.stored.RIExchangeMaxDailyUSD, "ri_exchange update survived") +} + func TestHandler_updateConfig_InvalidBody(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) diff --git a/internal/api/handler_ladder.go b/internal/api/handler_ladder.go new file mode 100644 index 000000000..c641134de --- /dev/null +++ b/internal/api/handler_ladder.go @@ -0,0 +1,136 @@ +package api + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/aws/aws-lambda-go/events" +) + +// ========================================== +// LADDER CONFIG HANDLERS +// ========================================== + +// getLadderConfigs returns all per-account ladder configurations. +// Requires view:config permission. +func (h *Handler) getLadderConfigs(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { + if _, err := h.requirePermission(ctx, req, "view", "config"); err != nil { + return nil, err + } + + configs, err := h.config.GetLadderConfigs(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list ladder configs: %w", err) + } + + return map[string]any{"configs": configs}, nil +} + +// upsertLadderConfig inserts or updates the per-account ladder configuration +// for the given (cloud_account_id, provider) pair extracted from the request +// body. Requires update:config permission. +// +// Auth dispatch mirrors handler_ri_exchange.go (session RBAC only; no token +// path exists for config writes). requirePermission denies the request when +// the auth component is nil (returns an error, never a session), so the +// handler fails closed; the exact status is a 500-class error in that case +// rather than a 403, but no unauthenticated write ever reaches the store. +func (h *Handler) upsertLadderConfig(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { + if _, err := h.requirePermission(ctx, req, "update", "config"); err != nil { + return nil, err + } + + // DisallowUnknownFields so a typo'd key is rejected loudly instead of + // silently dropped -- e.g. a mistyped "max_hourly_commit_per_run" would + // decode to nil, which the engine reads as "no spend cap" (unbounded). + var cfg config.LadderConfigDB + dec := json.NewDecoder(bytes.NewReader([]byte(req.Body))) + dec.DisallowUnknownFields() + if err := dec.Decode(&cfg); err != nil { + return nil, NewClientError(400, fmt.Sprintf("invalid request body: %s", err)) + } + + if cfg.CloudAccountID == "" { + return nil, NewClientError(400, "cloud_account_id is required") + } + if cfg.Provider == "" { + return nil, NewClientError(400, "provider is required") + } + + // Presence map to distinguish an OMITTED field from an explicitly-supplied + // value, so defaults apply only to genuinely-absent keys (see + // applyLadderConfigNumericDefaults). + var present map[string]json.RawMessage + if err := json.Unmarshal([]byte(req.Body), &present); err != nil { + return nil, NewClientError(400, "invalid request body") + } + applyLadderConfigNumericDefaults(&cfg, present) + + if err := cfg.Validate(); err != nil { + return nil, NewClientError(400, fmt.Sprintf("validation error: %s", err)) + } + + if err := h.validateLadderAccountProvider(ctx, &cfg); err != nil { + return nil, err + } + + result, err := h.config.UpsertLadderConfig(ctx, &cfg) + if err != nil { + return nil, fmt.Errorf("failed to upsert ladder config: %w", err) + } + + return result, nil +} + +// validateLadderAccountProvider confirms the (cloud_account_id, provider) pair +// refers to a real cloud account whose provider matches. This turns a +// nonexistent-account FK violation into a clean 400 (rather than a raw 500 from +// the store's FK constraint) and rejects a provider mismatch -- e.g. an inert +// gcp config attached to an AWS account, which would confuse PR-2 scoping. +func (h *Handler) validateLadderAccountProvider(ctx context.Context, cfg *config.LadderConfigDB) error { + account, err := h.config.GetCloudAccount(ctx, cfg.CloudAccountID) + if err != nil { + return fmt.Errorf("failed to look up cloud account: %w", err) + } + if account == nil { + return NewClientError(400, fmt.Sprintf("cloud account %q does not exist", cfg.CloudAccountID)) + } + if account.Provider != cfg.Provider { + return NewClientError(400, fmt.Sprintf("provider %q does not match cloud account provider %q", cfg.Provider, account.Provider)) + } + return nil +} + +// applyLadderConfigNumericDefaults fills a numeric field with its default ONLY +// when the field's key is genuinely absent from the request body (tracked via +// present). When a key IS supplied, the caller's value is left untouched so it +// flows to Validate(), which rejects any out-of-range value -- including an +// explicit 0 on the five fields whose valid range excludes 0 -- with a 400. A +// bare `== 0` check would instead silently rewrite an explicit out-of-range 0 +// to the default, the forbidden silent-fallback pattern on a money-adjacent +// config path (feedback_no_silent_fallbacks). buffer_fraction's valid range is +// [0, 1), so an explicit 0 there is a legitimate "no buffer" choice and is +// likewise passed through untouched. +func applyLadderConfigNumericDefaults(cfg *config.LadderConfigDB, present map[string]json.RawMessage) { + if _, ok := present["target_coverage"]; !ok { + cfg.TargetCoverage = config.DefaultLadderTargetCoverage + } + if _, ok := present["buffer_fraction"]; !ok { + cfg.BufferFraction = config.DefaultLadderBufferFraction + } + if _, ok := present["baseline_percentile"]; !ok { + cfg.BaselinePercentile = config.DefaultLadderBaselinePercentile + } + if _, ok := present["lookback_days"]; !ok { + cfg.LookbackDays = config.DefaultLadderLookbackDays + } + if _, ok := present["buffer_utilization_threshold"]; !ok { + cfg.BufferUtilizationThreshold = config.DefaultLadderBufferUtilThreshold + } + if _, ok := present["max_actions_per_run"]; !ok { + cfg.MaxActionsPerRun = config.DefaultLadderMaxActionsPerRun + } +} diff --git a/internal/api/handler_ladder_test.go b/internal/api/handler_ladder_test.go new file mode 100644 index 000000000..83afd5d9e --- /dev/null +++ b/internal/api/handler_ladder_test.go @@ -0,0 +1,169 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// ladderReq builds an authenticated PUT request carrying body. +func ladderReq(body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: body, + } +} + +// newLadderHandler wires a Handler with an admin session and returns the mock +// store plus a pointer that captures the config handed to UpsertLadderConfig +// (nil until the store is actually reached). +func newLadderHandler(t *testing.T) (*Handler, *MockConfigStore, **config.LadderConfigDB) { + t.Helper() + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) }) + + mockAuth.On("ValidateSession", ctx, "admin-token"). + Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + mockAuth.grantAdmin() + + var captured *config.LadderConfigDB + // .Maybe() so the 400-path test (store never reached) doesn't trip + // AssertExpectations on an unmet expectation. The success tests prove the + // store was reached by asserting *captured is non-nil (set only in Run). + mockStore.On("UpsertLadderConfig", ctx, mock.AnythingOfType("*config.LadderConfigDB")). + Run(func(args mock.Arguments) { + captured = args.Get(1).(*config.LadderConfigDB) + }). + Return(&config.LadderConfigDB{}, nil). + Maybe() + + return &Handler{config: mockStore, auth: mockAuth}, mockStore, &captured +} + +const ladderValidRamp = `"ramp_schedule":{"steps":[{"after_days":0,"fraction":1.0}]}` + +// TestUpsertLadderConfig_BufferFractionExplicitZeroSurvives asserts that an +// explicit buffer_fraction:0 ("no buffer") reaches the store unchanged rather +// than being silently rewritten to DefaultLadderBufferFraction. +func TestUpsertLadderConfig_BufferFractionExplicitZeroSurvives(t *testing.T) { + ctx := context.Background() + handler, _, captured := newLadderHandler(t) + + body := `{"cloud_account_id":"acct-1","provider":"aws","mode":"email_approval","cadence":"daily","buffer_fraction":0,` + ladderValidRamp + `}` + _, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.NoError(t, err) + require.NotNil(t, *captured, "store should have been reached") + assert.Equal(t, 0.0, (*captured).BufferFraction, + "explicit buffer_fraction:0 must survive to the store unchanged") +} + +// TestUpsertLadderConfig_BufferFractionOmittedDefaults asserts that an omitted +// buffer_fraction key defaults to DefaultLadderBufferFraction. +func TestUpsertLadderConfig_BufferFractionOmittedDefaults(t *testing.T) { + ctx := context.Background() + handler, _, captured := newLadderHandler(t) + + body := `{"cloud_account_id":"acct-1","provider":"aws","mode":"email_approval","cadence":"daily",` + ladderValidRamp + `}` + _, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.NoError(t, err) + require.NotNil(t, *captured, "store should have been reached") + assert.Equal(t, config.DefaultLadderBufferFraction, (*captured).BufferFraction, + "omitted buffer_fraction must default to DefaultLadderBufferFraction") +} + +// TestUpsertLadderConfig_ExplicitZeroTargetCoverageRejected asserts the FIX 1 +// contract: an explicit out-of-range target_coverage:0 returns 400 and never +// reaches the store (it is not silently defaulted). +func TestUpsertLadderConfig_ExplicitZeroTargetCoverageRejected(t *testing.T) { + ctx := context.Background() + handler, mockStore, _ := newLadderHandler(t) + + body := `{"cloud_account_id":"acct-1","provider":"aws","mode":"email_approval","cadence":"daily","target_coverage":0,` + ladderValidRamp + `}` + result, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + mockStore.AssertNotCalled(t, "UpsertLadderConfig", mock.Anything, mock.Anything) +} + +// TestUpsertLadderConfig_MultiStepRampAccepted is the F1 end-to-end regression: +// a multi-step ramp (after_days 0 -> 30 -> 60) must be ACCEPTED and round-trip +// to the store unchanged. Against the pre-tag pkg/ladder code every AfterDays +// decoded to 0, so Validate rejected the ramp (not strictly ascending) and this +// PUT returned 400. +func TestUpsertLadderConfig_MultiStepRampAccepted(t *testing.T) { + ctx := context.Background() + handler, _, captured := newLadderHandler(t) + + const ramp = `"ramp_schedule":{"steps":[{"after_days":0,"fraction":0.5},{"after_days":30,"fraction":0.3},{"after_days":60,"fraction":0.2}]}` + body := `{"cloud_account_id":"acct-1","provider":"aws","mode":"email_approval","cadence":"daily",` + ramp + `}` + result, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.NoError(t, err) + require.NotNil(t, result) + require.NotNil(t, *captured, "store should have been reached") + // The multi-step ramp round-trips to the store unchanged (raw JSONB). + assert.JSONEq(t, + `{"steps":[{"after_days":0,"fraction":0.5},{"after_days":30,"fraction":0.3},{"after_days":60,"fraction":0.2}]}`, + string((*captured).RampSchedule)) +} + +// TestUpsertLadderConfig_UnknownFieldRejected is F5: a typo'd key must be +// rejected with 400 (DisallowUnknownFields), not silently dropped -- a mistyped +// max_hourly_commit_per_run would otherwise decode to nil = no spend cap. +func TestUpsertLadderConfig_UnknownFieldRejected(t *testing.T) { + ctx := context.Background() + handler, mockStore, _ := newLadderHandler(t) + + body := `{"cloud_account_id":"acct-1","provider":"aws","mode":"email_approval","cadence":"daily","max_hourly_commit_per_runn":5,` + ladderValidRamp + `}` + result, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + mockStore.AssertNotCalled(t, "UpsertLadderConfig", mock.Anything, mock.Anything) +} + +// TestUpsertLadderConfig_ProviderMismatchRejected is F5: the request provider +// must match the cloud account's actual provider. The default GetCloudAccount +// mock returns provider "aws"; a body claiming "azure" must 400. +func TestUpsertLadderConfig_ProviderMismatchRejected(t *testing.T) { + ctx := context.Background() + handler, mockStore, _ := newLadderHandler(t) + + body := `{"cloud_account_id":"acct-1","provider":"azure","mode":"email_approval","cadence":"daily",` + ladderValidRamp + `}` + result, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + mockStore.AssertNotCalled(t, "UpsertLadderConfig", mock.Anything, mock.Anything) +} + +// TestUpsertLadderConfig_NonexistentAccountRejected is F5: a cloud_account_id +// that does not resolve must 400 (clean rejection) rather than reaching the +// store and surfacing a raw 500 from the FK constraint. +func TestUpsertLadderConfig_NonexistentAccountRejected(t *testing.T) { + ctx := context.Background() + handler, mockStore, _ := newLadderHandler(t) + mockStore.On("GetCloudAccount", ctx, "ghost").Return(nil, nil) + + body := `{"cloud_account_id":"ghost","provider":"aws","mode":"email_approval","cadence":"daily",` + ladderValidRamp + `}` + result, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + mockStore.AssertNotCalled(t, "UpsertLadderConfig", mock.Anything, mock.Anything) +} diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 2f4d72250..635dc324a 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -927,19 +927,23 @@ func (h *Handler) updateRIExchangeConfig(ctx context.Context, req *events.Lambda return nil, err } - globalCfg, err := h.config.GetGlobalConfig(ctx) - if err != nil { - return nil, fmt.Errorf("failed to load config: %w", err) - } - - globalCfg.RIExchangeEnabled = body.AutoExchangeEnabled - globalCfg.RIExchangeMode = body.Mode - globalCfg.RIExchangeUtilizationThreshold = body.UtilizationThreshold - globalCfg.RIExchangeMaxPerExchangeUSD = body.MaxPaymentPerExchangeUSD - globalCfg.RIExchangeMaxDailyUSD = body.MaxPaymentDailyUSD - globalCfg.RIExchangeLookbackDays = body.LookbackDays - - if err := h.config.SaveGlobalConfig(ctx, globalCfg); err != nil { + // Route through the serialized read-modify-write so this partial update + // (only the ri_exchange_* columns) shares the advisory lock with other + // global_config writers. An unlocked GetGlobalConfig -> mutate -> + // SaveGlobalConfig here would write all 21 columns and could silently + // revert a concurrent kill-switch toggle done via UpdateGlobalConfigAtomic. + // The closure mutates ONLY the ri_exchange_* fields; body.validate above + // already validated the inputs, so (matching the prior behavior) we do not + // re-run the whole-config Validate here. + if _, err := h.config.UpdateGlobalConfigAtomic(ctx, func(existing *config.GlobalConfig) error { + existing.RIExchangeEnabled = body.AutoExchangeEnabled + existing.RIExchangeMode = body.Mode + existing.RIExchangeUtilizationThreshold = body.UtilizationThreshold + existing.RIExchangeMaxPerExchangeUSD = body.MaxPaymentPerExchangeUSD + existing.RIExchangeMaxDailyUSD = body.MaxPaymentDailyUSD + existing.RIExchangeLookbackDays = body.LookbackDays + return nil + }); err != nil { return nil, fmt.Errorf("failed to save config: %w", err) } diff --git a/internal/api/router.go b/internal/api/router.go index e41f9e406..f69a00dc6 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -308,6 +308,13 @@ func (r *Router) registerRoutes() { {PathPrefix: "/api/ri-exchange/approve/", Method: "POST", Handler: r.approveRIExchangeHandler, Auth: AuthPublic}, {PathPrefix: "/api/ri-exchange/reject/", Method: "POST", Handler: r.rejectRIExchangeHandler, Auth: AuthPublic}, + // Commitment Laddering endpoints (flag-gated default-off, issue #1336). + // GET returns all per-account ladder configs; PUT inserts or updates one. + // Both routes require update:config / view:config (checked inside the + // handler), consistent with the RI Exchange config precedent. + {ExactPath: "/api/ladder/configs", Method: "GET", Handler: r.getLadderConfigsHandler, Auth: AuthUser}, + {ExactPath: "/api/ladder/configs", Method: "PUT", Handler: r.upsertLadderConfigHandler, Auth: AuthUser}, + // Account self-registration (public, called by Terraform during federation IaC apply) {ExactPath: "/api/register", Method: "POST", Handler: r.submitRegistrationHandler, Auth: AuthPublic}, {PathPrefix: "/api/register/", Method: "GET", Handler: r.getRegistrationStatusHandler, Auth: AuthPublic}, @@ -906,3 +913,13 @@ func (r *Router) listPlanAccountsHandler(ctx context.Context, req *events.Lambda func (r *Router) setPlanAccountsHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { return r.h.setPlanAccounts(ctx, req, params["id"]) } + +// Commitment Laddering route wrappers (issue #1336). + +func (r *Router) getLadderConfigsHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { + return r.h.getLadderConfigs(ctx, req) +} + +func (r *Router) upsertLadderConfigHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { + return r.h.upsertLadderConfig(ctx, req) +} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 51b2bf5ef..0ed2cd7c2 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -12,6 +12,11 @@ type StoreInterface interface { // Global configuration GetGlobalConfig(ctx context.Context) (*GlobalConfig, error) SaveGlobalConfig(ctx context.Context, config *GlobalConfig) error + // UpdateGlobalConfigAtomic serializes a read-modify-write of the + // global_config singleton under an advisory-locked transaction so + // concurrent partial PUTs cannot lose each other's updates. apply mutates + // the loaded config in place; its error aborts the write and is propagated. + UpdateGlobalConfigAtomic(ctx context.Context, apply func(*GlobalConfig) error) (*GlobalConfig, error) // Service configuration GetServiceConfig(ctx context.Context, provider, service string) (*ServiceConfig, error) @@ -314,4 +319,14 @@ type StoreInterface interface { // participate in the transaction. Nested transactions are not // supported — fn must not call WithTx recursively. WithTx(ctx context.Context, fn func(tx pgx.Tx) error) error + + // Ladder configuration (per-account, per-provider). + // GetLadderConfigs returns all rows, newest first. + // GetLadderConfig returns the single row for (cloudAccountID, provider), + // or (nil, nil) when no row exists. + // UpsertLadderConfig inserts or updates via the UNIQUE(cloud_account_id, provider) + // constraint and returns the persisted row with all DB-stamped fields populated. + GetLadderConfigs(ctx context.Context) ([]LadderConfigDB, error) + GetLadderConfig(ctx context.Context, cloudAccountID, provider string) (*LadderConfigDB, error) + UpsertLadderConfig(ctx context.Context, cfg *LadderConfigDB) (*LadderConfigDB, error) } diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 5317234b4..04d832758 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "hash/fnv" "sort" "strings" "time" @@ -42,8 +43,33 @@ var _ StoreInterface = (*PostgresStore)(nil) // GLOBAL CONFIGURATION // ========================================== -// GetGlobalConfig retrieves the global configuration +// globalConfigExecutor is the subset of query methods needed to read and write +// the global_config singleton. It is satisfied by both the pool (dbConn) and a +// pgx.Tx, so the read/write helpers can run either on the pool directly or +// inside the locked transaction opened by UpdateGlobalConfigAtomic. +type globalConfigExecutor interface { + QueryRow(ctx context.Context, sql string, args ...any) pgx.Row + Exec(ctx context.Context, sql string, args ...any) (pgconn.CommandTag, error) +} + +// globalConfigLockKey is the advisory-lock key that serializes concurrent +// read-modify-write updates to the global_config singleton (updateConfig +// partial PUTs). Derived like the scheduled-task locks (FNV-64a of a +// namespaced string) so it cannot collide with those or the migration lock. +var globalConfigLockKey = func() int64 { + h := fnv.New64a() + _, _ = h.Write([]byte("cudly:global_config:singleton")) + return int64(h.Sum64()) +}() + +// GetGlobalConfig retrieves the global configuration. func (s *PostgresStore) GetGlobalConfig(ctx context.Context) (*GlobalConfig, error) { + return getGlobalConfigFrom(ctx, s.db) +} + +// getGlobalConfigFrom reads the global_config singleton via q (the pool or a +// transaction), returning defaults when no row exists yet. +func getGlobalConfigFrom(ctx context.Context, q globalConfigExecutor) (*GlobalConfig, error) { query := ` SELECT enabled_providers, notification_email, approval_required, default_term, default_payment, default_coverage, default_ramp_schedule, @@ -52,7 +78,8 @@ func (s *PostgresStore) GetGlobalConfig(ctx context.Context) (*GlobalConfig, err auto_collect, collection_schedule, notification_days_before, grace_period_days, recommendations_cache_stale_hours, recommendations_lookback_days, - COALESCE(purchase_delay_hours, 0) + COALESCE(purchase_delay_hours, 0), + COALESCE(laddering_enabled, false) FROM global_config WHERE id = 1 ` @@ -61,7 +88,7 @@ func (s *PostgresStore) GetGlobalConfig(ctx context.Context) (*GlobalConfig, err var enabledProviders []string var gracePeriodJSON string - err := s.db.QueryRow(ctx, query).Scan( + err := q.QueryRow(ctx, query).Scan( &enabledProviders, &config.NotificationEmail, &config.ApprovalRequired, @@ -82,10 +109,11 @@ func (s *PostgresStore) GetGlobalConfig(ctx context.Context) (*GlobalConfig, err &config.RecommendationsCacheStaleHours, &config.RecommendationsLookbackDays, &config.PurchaseDelayHours, + &config.LadderingEnabled, ) if err != nil { - if err == pgx.ErrNoRows { + if errors.Is(err, pgx.ErrNoRows) { // Return default config if none exists. // Values must align with DefaultSettings in defaults.go and DB DEFAULT clauses. return &GlobalConfig{ @@ -120,8 +148,52 @@ func (s *PostgresStore) GetGlobalConfig(ctx context.Context) (*GlobalConfig, err return &config, nil } -// SaveGlobalConfig saves the global configuration +// SaveGlobalConfig saves the global configuration. func (s *PostgresStore) SaveGlobalConfig(ctx context.Context, config *GlobalConfig) error { + return saveGlobalConfigWith(ctx, s.db, config) +} + +// UpdateGlobalConfigAtomic performs a serialized read-modify-write of the +// global_config singleton. It opens a transaction, takes a transaction-scoped +// advisory lock (which serializes even the first insert, since a row-level +// FOR UPDATE cannot lock a not-yet-existing singleton row), reads the current +// config, applies the caller's in-place mutation, and upserts the result in +// the SAME transaction. This eliminates the lost-update race where two +// concurrent partial PUTs each read the same stale base and the later save +// silently drops the earlier change. +// +// apply mutates the loaded config in place (e.g. json.Unmarshal the request +// body over it and validate); an error it returns aborts the transaction and +// is propagated unchanged (so callers can surface a 400 from validation while +// transport/DB errors surface as 500). +func (s *PostgresStore) UpdateGlobalConfigAtomic(ctx context.Context, apply func(*GlobalConfig) error) (*GlobalConfig, error) { + var merged *GlobalConfig + err := s.WithTx(ctx, func(tx pgx.Tx) error { + if _, lockErr := tx.Exec(ctx, "SELECT pg_advisory_xact_lock($1)", globalConfigLockKey); lockErr != nil { + return fmt.Errorf("failed to acquire global_config advisory lock: %w", lockErr) + } + cfg, readErr := getGlobalConfigFrom(ctx, tx) + if readErr != nil { + return readErr + } + if applyErr := apply(cfg); applyErr != nil { + return applyErr + } + if saveErr := saveGlobalConfigWith(ctx, tx, cfg); saveErr != nil { + return saveErr + } + merged = cfg + return nil + }) + if err != nil { + return nil, err + } + return merged, nil +} + +// saveGlobalConfigWith upserts the global_config singleton via q (the pool or a +// transaction). +func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *GlobalConfig) error { // Ensure EnabledProviders is never nil (empty slice is ok, nil is not) if config.EnabledProviders == nil { config.EnabledProviders = []string{} @@ -136,8 +208,8 @@ func (s *PostgresStore) SaveGlobalConfig(ctx context.Context, config *GlobalConf auto_collect, collection_schedule, notification_days_before, grace_period_days, recommendations_cache_stale_hours, recommendations_lookback_days, - purchase_delay_hours - ) VALUES (1, $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20) + purchase_delay_hours, laddering_enabled + ) VALUES (1, $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21) ON CONFLICT (id) DO UPDATE SET enabled_providers = $1, notification_email = $2, @@ -159,6 +231,7 @@ func (s *PostgresStore) SaveGlobalConfig(ctx context.Context, config *GlobalConf recommendations_cache_stale_hours = $18, recommendations_lookback_days = $19, purchase_delay_hours = $20, + laddering_enabled = $21, updated_at = NOW() ` @@ -194,7 +267,7 @@ func (s *PostgresStore) SaveGlobalConfig(ctx context.Context, config *GlobalConf gracePeriodJSON = string(gpBytes) } - _, err := s.db.Exec(ctx, query, + _, err := q.Exec(ctx, query, config.EnabledProviders, config.NotificationEmail, config.ApprovalRequired, @@ -215,6 +288,7 @@ func (s *PostgresStore) SaveGlobalConfig(ctx context.Context, config *GlobalConf config.RecommendationsCacheStaleHours, recommendationsLookbackDays, config.PurchaseDelayHours, + config.LadderingEnabled, ) if err != nil { diff --git a/internal/config/store_postgres_ladder.go b/internal/config/store_postgres_ladder.go new file mode 100644 index 000000000..5d3290312 --- /dev/null +++ b/internal/config/store_postgres_ladder.go @@ -0,0 +1,192 @@ +package config + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5" +) + +// ========================================== +// LADDER CONFIG STORE METHODS +// ========================================== + +// GetLadderConfigs returns all ladder_configs rows, newest first. +// Returns an empty slice (not nil) when no rows exist. +func (s *PostgresStore) GetLadderConfigs(ctx context.Context) ([]LadderConfigDB, error) { + query := ` + SELECT id, cloud_account_id, provider, enabled, mode, cadence, + target_coverage, buffer_fraction, baseline_percentile, + lookback_days, buffer_utilization_threshold, + max_hourly_commit_per_run, max_actions_per_run, + ramp_schedule, created_at, updated_at + FROM ladder_configs + ORDER BY created_at DESC + ` + rows, err := s.db.Query(ctx, query) + if err != nil { + return nil, fmt.Errorf("failed to query ladder_configs: %w", err) + } + defer rows.Close() + + var configs []LadderConfigDB + for rows.Next() { + cfg, err := scanLadderConfig(rows) + if err != nil { + return nil, fmt.Errorf("failed to scan ladder_config row: %w", err) + } + configs = append(configs, cfg) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("error iterating ladder_config rows: %w", err) + } + if configs == nil { + configs = []LadderConfigDB{} + } + return configs, nil +} + +// GetLadderConfig returns the ladder_config row for the given +// (cloud_account_id, provider) pair. Returns (nil, nil) when no row exists. +func (s *PostgresStore) GetLadderConfig(ctx context.Context, cloudAccountID, provider string) (*LadderConfigDB, error) { + query := ` + SELECT id, cloud_account_id, provider, enabled, mode, cadence, + target_coverage, buffer_fraction, baseline_percentile, + lookback_days, buffer_utilization_threshold, + max_hourly_commit_per_run, max_actions_per_run, + ramp_schedule, created_at, updated_at + FROM ladder_configs + WHERE cloud_account_id = $1 AND provider = $2 + ` + row := s.db.QueryRow(ctx, query, cloudAccountID, provider) + cfg, err := scanLadderConfig(row) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, nil + } + return nil, fmt.Errorf("failed to get ladder_config for account=%s provider=%s: %w", cloudAccountID, provider, err) + } + return &cfg, nil +} + +// UpsertLadderConfig inserts or updates the per-account ladder configuration. +// The upsert key is (cloud_account_id, provider). If ID is empty a new UUID +// is generated; existing rows retain their original id and created_at. +// +// Validate() must be called by the API handler before this method; the store +// does not re-validate to avoid duplicating error messages. +func (s *PostgresStore) UpsertLadderConfig(ctx context.Context, cfg *LadderConfigDB) (*LadderConfigDB, error) { + if cfg.ID == "" { + cfg.ID = uuid.New().String() + } + + rampJSON, err := marshalRampSchedule(cfg.RampSchedule) + if err != nil { + return nil, fmt.Errorf("failed to marshal ramp_schedule: %w", err) + } + + query := ` + INSERT INTO ladder_configs ( + id, cloud_account_id, provider, enabled, mode, cadence, + target_coverage, buffer_fraction, baseline_percentile, + lookback_days, buffer_utilization_threshold, + max_hourly_commit_per_run, max_actions_per_run, + ramp_schedule, created_at, updated_at + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, NOW(), NOW()) + ON CONFLICT (cloud_account_id, provider) DO UPDATE SET + enabled = $4, + mode = $5, + cadence = $6, + target_coverage = $7, + buffer_fraction = $8, + baseline_percentile = $9, + lookback_days = $10, + buffer_utilization_threshold = $11, + max_hourly_commit_per_run = $12, + max_actions_per_run = $13, + ramp_schedule = $14, + updated_at = NOW() + RETURNING id, cloud_account_id, provider, enabled, mode, cadence, + target_coverage, buffer_fraction, baseline_percentile, + lookback_days, buffer_utilization_threshold, + max_hourly_commit_per_run, max_actions_per_run, + ramp_schedule, created_at, updated_at + ` + + row := s.db.QueryRow(ctx, query, + cfg.ID, + cfg.CloudAccountID, + cfg.Provider, + cfg.Enabled, + cfg.Mode, + cfg.Cadence, + cfg.TargetCoverage, + cfg.BufferFraction, + cfg.BaselinePercentile, + cfg.LookbackDays, + cfg.BufferUtilizationThreshold, + cfg.MaxHourlyCommitPerRun, + cfg.MaxActionsPerRun, + rampJSON, + ) + result, err := scanLadderConfig(row) + if err != nil { + return nil, fmt.Errorf("failed to upsert ladder_config for account=%s provider=%s: %w", + cfg.CloudAccountID, cfg.Provider, err) + } + return &result, nil +} + +// scanLadderConfig scans a single ladder_configs row from either a pgx.Row +// or pgx.Rows. Both types satisfy the scannable interface (declared in +// store_postgres_registrations.go). This helper exists to avoid duplicating +// the 16-column scan logic. +func scanLadderConfig(row scannable) (LadderConfigDB, error) { + var cfg LadderConfigDB + var rampJSON []byte + var createdAt, updatedAt time.Time + + err := row.Scan( + &cfg.ID, + &cfg.CloudAccountID, + &cfg.Provider, + &cfg.Enabled, + &cfg.Mode, + &cfg.Cadence, + &cfg.TargetCoverage, + &cfg.BufferFraction, + &cfg.BaselinePercentile, + &cfg.LookbackDays, + &cfg.BufferUtilizationThreshold, + &cfg.MaxHourlyCommitPerRun, + &cfg.MaxActionsPerRun, + &rampJSON, + &createdAt, + &updatedAt, + ) + if err != nil { + return LadderConfigDB{}, err + } + cfg.CreatedAt = createdAt + cfg.UpdatedAt = updatedAt + cfg.RampSchedule = json.RawMessage(rampJSON) + return cfg, nil +} + +// marshalRampSchedule ensures the ramp_schedule value stored in the DB is +// valid JSON. Nil/empty input is rejected; already-valid JSON is passed +// through unchanged. Non-JSON input is rejected with a descriptive error. +func marshalRampSchedule(raw json.RawMessage) ([]byte, error) { + if len(raw) == 0 { + return nil, fmt.Errorf("ramp_schedule must not be empty") + } + // Validate it is parseable JSON before writing. + if !json.Valid(raw) { + return nil, fmt.Errorf("ramp_schedule is not valid JSON") + } + return raw, nil +} diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index ae157c24a..cebe69d08 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -57,6 +57,7 @@ func TestPGXMock_GetGlobalConfig_Success(t *testing.T) { "grace_period_days", "recommendations_cache_stale_hours", "recommendations_lookback_days", "purchase_delay_hours", + "laddering_enabled", } rows := pgxmock.NewRows(cols).AddRow( []string{"aws"}, strPtr("ops@example.com"), true, @@ -67,6 +68,7 @@ func TestPGXMock_GetGlobalConfig_Success(t *testing.T) { "{}", 24, 7, 0, + false, ) mock.ExpectQuery("SELECT").WillReturnRows(rows) @@ -107,6 +109,7 @@ func TestPGXMock_GetGlobalConfig_GracePeriodDays(t *testing.T) { "grace_period_days", "recommendations_cache_stale_hours", "recommendations_lookback_days", "purchase_delay_hours", + "laddering_enabled", } baseRow := func(graceJSON string) []any { return []any{ @@ -118,6 +121,7 @@ func TestPGXMock_GetGlobalConfig_GracePeriodDays(t *testing.T) { graceJSON, 24, 7, 0, + false, } } @@ -163,6 +167,118 @@ func TestPGXMock_GetGlobalConfig_GracePeriodDays(t *testing.T) { }) } +// globalConfigCols is the column list returned by the GetGlobalConfig SELECT, +// in scan order. +var globalConfigCols = []string{ + "enabled_providers", "notification_email", "approval_required", + "default_term", "default_payment", "default_coverage", "default_ramp_schedule", + "ri_exchange_enabled", "ri_exchange_mode", "ri_exchange_utilization_threshold", + "ri_exchange_max_per_exchange_usd", "ri_exchange_max_daily_usd", "ri_exchange_lookback_days", + "auto_collect", "collection_schedule", "notification_days_before", + "grace_period_days", + "recommendations_cache_stale_hours", "recommendations_lookback_days", + "purchase_delay_hours", + "laddering_enabled", +} + +// TestPGXMock_UpdateGlobalConfigAtomic_LockedReadModifyWrite proves the F2 +// lost-update fix: UpdateGlobalConfigAtomic performs the read and the write in +// ONE transaction, guarded by a transaction-scoped advisory lock, in strict +// order: BEGIN -> pg_advisory_xact_lock -> SELECT global_config -> UPSERT -> +// COMMIT. Because pgxmock enforces expectation ORDER, a passing run guarantees +// the read-modify-write is serialized under the lock rather than split across +// two unsynchronized statements (the original TOCTOU). The apply closure +// mutates only one field; the round-tripped result must keep every field the +// closure did not touch, i.e. a concurrent partial update cannot lose another's +// change once the lock serializes them. +func TestPGXMock_UpdateGlobalConfigAtomic_LockedReadModifyWrite(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + // Existing row carries automation settings a partial PUT must not clobber. + seeded := pgxmock.NewRows(globalConfigCols).AddRow( + []string{"aws"}, strPtr("ops@example.com"), true, // approval_required = true + 3, "all-upfront", 80.0, RampImmediate, + true, "automatic", 95.0, // ri_exchange_enabled = true, mode = automatic + 1000.0, 5000.0, 30, + true, "daily", 3, + "{}", + 24, 7, + 48, + false, // laddering_enabled = false + ) + + // Strict order: the SELECT and the UPSERT must sit between the same + // BEGIN/COMMIT, after the advisory lock. + mock.ExpectBegin() + mock.ExpectExec("pg_advisory_xact_lock").WithArgs(pgxmock.AnyArg()). + WillReturnResult(pgxmock.NewResult("SELECT", 1)) + mock.ExpectQuery("FROM global_config").WillReturnRows(seeded) + mock.ExpectExec("INSERT INTO global_config").WithArgs(anyArgsCfg(21)...). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit() + + // apply flips only laddering_enabled, exactly as the kill-switch partial + // PUT does; everything else must be preserved from the seeded row. + applied := false + merged, err := store.UpdateGlobalConfigAtomic(ctx, func(existing *GlobalConfig) error { + applied = true + existing.LadderingEnabled = true + return nil + }) + require.NoError(t, err) + require.True(t, applied, "apply closure must run against the locked-read config") + require.NotNil(t, merged) + + // Applied change survives. + assert.True(t, merged.LadderingEnabled) + // Fields the closure did NOT touch are preserved (no lost update). + assert.True(t, merged.ApprovalRequired, "approval_required must survive") + assert.True(t, merged.RIExchangeEnabled, "ri_exchange_enabled must survive") + assert.Equal(t, "automatic", merged.RIExchangeMode) + assert.Equal(t, 1000.0, merged.RIExchangeMaxPerExchangeUSD) + assert.Equal(t, 48, merged.PurchaseDelayHours) + + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_UpdateGlobalConfigAtomic_ApplyErrorRollsBack asserts that when the +// apply closure fails (e.g. validation), the transaction rolls back and no +// UPSERT is issued, and the closure's error propagates unchanged. +func TestPGXMock_UpdateGlobalConfigAtomic_ApplyErrorRollsBack(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + seeded := pgxmock.NewRows(globalConfigCols).AddRow( + []string{"aws"}, (*string)(nil), true, + 3, "all-upfront", 80.0, RampImmediate, + false, "manual", 95.0, + 0.0, 0.0, 30, + true, "daily", 3, + "{}", + 24, 7, + 0, + false, + ) + + mock.ExpectBegin() + mock.ExpectExec("pg_advisory_xact_lock").WithArgs(pgxmock.AnyArg()). + WillReturnResult(pgxmock.NewResult("SELECT", 1)) + mock.ExpectQuery("FROM global_config").WillReturnRows(seeded) + // No ExpectExec("INSERT...") — the UPSERT must not run. + mock.ExpectRollback() + + sentinel := errors.New("validation failed") + _, err := store.UpdateGlobalConfigAtomic(ctx, func(_ *GlobalConfig) error { + return sentinel + }) + require.Error(t, err) + assert.ErrorIs(t, err, sentinel) + assert.NoError(t, mock.ExpectationsWereMet()) +} + // ─── GetServiceConfig ───────────────────────────────────────────────────────── func TestPGXMock_GetServiceConfig_Success(t *testing.T) { diff --git a/internal/config/types.go b/internal/config/types.go index 418ce95a9..962bcce76 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -5,6 +5,8 @@ import ( "encoding/json" "strings" "time" + + "github.com/LeanerCloud/CUDly/pkg/ladder" ) // GlobalConfig represents the global CUDly configuration @@ -61,6 +63,12 @@ type GlobalConfig struct { // 0 means immediate-execute (backward compat). Valid range: [0, 168]. // Default: 48. PurchaseDelayHours int `json:"purchase_delay_hours" db:"purchase_delay_hours"` + + // LadderingEnabled is the global kill-switch for the commitment-laddering + // feature (issue #1333 phase 3). When false (the default), no laddering + // engine runs fire regardless of per-account LadderConfig.Enabled settings. + // Set to true to allow per-account configs to activate individually. + LadderingEnabled bool `json:"laddering_enabled" db:"laddering_enabled"` } // DefaultGracePeriodDays is the fallback window used when a provider @@ -930,3 +938,79 @@ type AccountRegistrationFilter struct { Provider *string Search string } + +// ========================================== +// COMMITMENT LADDERING +// ========================================== + +// Ladder default constants. These alias the canonical values in pkg/ladder so +// that internal/config callers can reference them by name without importing +// pkg/ladder directly. The string-typed mode/cadence values are also sourced +// from pkg/ladder to guarantee the DB and the engine always use the same tokens. + +// DefaultLadderTargetCoverage is the default commitment coverage target (%). +// Aliases ladder.DefaultTargetCoveragePct so only one source of truth exists. +const DefaultLadderTargetCoverage = ladder.DefaultTargetCoveragePct + +// DefaultLadderBufferFraction is the default fraction of the base allocation +// reserved in short-term / convertible buffer commitments. +const DefaultLadderBufferFraction = ladder.DefaultBufferFraction + +// DefaultLadderBaselinePercentile is the default usage percentile used to +// anchor the base commitment layer. +const DefaultLadderBaselinePercentile = ladder.DefaultBaselinePercentile + +// DefaultLadderLookbackDays is the default historical window (days) used to +// compute the usage baseline. +const DefaultLadderLookbackDays = ladder.DefaultLookbackDays + +// DefaultLadderBufferUtilThreshold is the default buffer-layer utilization % +// below which the engine emits a reshape recommendation. +const DefaultLadderBufferUtilThreshold = ladder.DefaultBufferUtilizationThresholdPct + +// DefaultLadderMaxActionsPerRun is the default cap on the number of +// PlannedActions the engine may execute per run. +const DefaultLadderMaxActionsPerRun = 10 + +// MaxLadderActionsPerRun is the ceiling enforced at validation time. A value +// above this is almost certainly a misconfiguration and should fail loud rather +// than fan out unbounded actions. +const MaxLadderActionsPerRun = 50 + +// LadderConfigDB is the DB-persistence mirror of pkg/ladder.LadderConfig. +// It stores one per-account, per-provider ladder configuration row and is +// used by the store layer (GetLadderConfig / UpsertLadderConfig) and the API +// handler. Mode and Cadence are plain strings whose valid values are defined +// by pkg/ladder (ModeEmailApproval, ModeAutoApprove, CadenceDaily, +// CadenceWeekly). Validation calls pkg/ladder's Parse* functions so the +// internal/config package never redefines those constants. +// +// MaxHourlyCommitPerRun is a pointer because nil means "no cap" (distinct from +// 0, which would cap all spending). All numeric money fields follow the project +// rule: absent = nil/pointer, never 0. +// Field order is optimized for govet fieldalignment (pointer-containing fields +// grouped first to shrink the GC pointer-scan range, then scalars, bool last). +// It intentionally does not follow the logical/SQL column order; see the +// scanLadderConfig / Upsert SQL for the wire order. +type LadderConfigDB struct { + UpdatedAt time.Time `json:"updated_at"` + CreatedAt time.Time `json:"created_at"` + // MaxHourlyCommitPerRun caps the total hourly commitment delta a single run + // may purchase. nil means no cap. + MaxHourlyCommitPerRun *float64 `json:"max_hourly_commit_per_run,omitempty"` + CloudAccountID string `json:"cloud_account_id"` + Provider string `json:"provider"` + Mode string `json:"mode"` // ladder.ModeEmailApproval | ladder.ModeAutoApprove + Cadence string `json:"cadence"` // ladder.CadenceDaily | ladder.CadenceWeekly + ID string `json:"id"` + RampSchedule json.RawMessage `json:"ramp_schedule"` + BufferUtilizationThreshold float64 `json:"buffer_utilization_threshold"` + LookbackDays int `json:"lookback_days"` + MaxActionsPerRun int `json:"max_actions_per_run"` + // BaselinePercentile is the statistical percentile used to anchor the base + // commitment layer. Must be in (0, 50]. + BaselinePercentile float64 `json:"baseline_percentile"` + BufferFraction float64 `json:"buffer_fraction"` + TargetCoverage float64 `json:"target_coverage"` + Enabled bool `json:"enabled"` +} diff --git a/internal/config/validation.go b/internal/config/validation.go index 58c0afd29..36f63c23a 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -2,10 +2,14 @@ package config import ( + "encoding/json" "fmt" + "math" "net/mail" "sort" "strings" + + "github.com/LeanerCloud/CUDly/pkg/ladder" ) // ValidProviders lists all supported cloud providers @@ -546,3 +550,110 @@ func ValidateRampScheduleEnv(val string) error { } return nil } + +// ========================================== +// LADDER CONFIG VALIDATION +// ========================================== + +// Validate validates a LadderConfigDB before persist. It delegates mode and +// cadence checks to pkg/ladder's Parse* functions (single source of truth for +// those enums) and validates numeric bounds using the same invariants that +// pkg/ladder.LadderConfig.Validate() enforces. +// +// Returns a specific, descriptive error on any violation. Callers must never +// silently default away a validation failure. +func (c *LadderConfigDB) Validate() error { + if err := validateLadderProvider(c.Provider); err != nil { + return err + } + if _, err := ladder.ParseLadderMode(c.Mode); err != nil { + return fmt.Errorf("mode: %w", err) + } + if _, err := ladder.ParseLadderCadence(c.Cadence); err != nil { + return fmt.Errorf("cadence: %w", err) + } + if err := c.validateLadderBaselineBounds(); err != nil { + return err + } + if err := c.validateLadderRunLimits(); err != nil { + return err + } + return c.validateLadderRampSchedule() +} + +// validateLadderProvider checks that Provider is a recognized cloud provider slug. +func validateLadderProvider(provider string) error { + for _, p := range ValidProviders { + if provider == p { + return nil + } + } + return fmt.Errorf("provider %q is not valid (allowed: %s)", provider, strings.Join(ValidProviders, ", ")) +} + +// validateLadderBaselineBounds checks the coverage target, buffer fraction, +// and baseline measurement parameters. Uses the same NaN-hostile invariants +// as pkg/ladder.LadderConfig.validateBaselineBounds (mirrored here so +// internal/config does not call unexported pkg methods). +func (c *LadderConfigDB) validateLadderBaselineBounds() error { + if err := c.validateLadderCoverageBounds(); err != nil { + return err + } + if math.IsNaN(c.BaselinePercentile) || !(c.BaselinePercentile > 0 && c.BaselinePercentile <= 50) { + return fmt.Errorf("baseline_percentile %g must be in (0, 50]", c.BaselinePercentile) + } + if c.LookbackDays <= 0 { + return fmt.Errorf("lookback_days %d must be > 0", c.LookbackDays) + } + return nil +} + +// validateLadderCoverageBounds checks the coverage target and buffer fraction. +// Split out from validateLadderBaselineBounds to keep each function under the +// cyclomatic-complexity gate. +func (c *LadderConfigDB) validateLadderCoverageBounds() error { + if math.IsNaN(c.TargetCoverage) || !(c.TargetCoverage > 0 && c.TargetCoverage <= 100) { + return fmt.Errorf("target_coverage %g must be in (0, 100]", c.TargetCoverage) + } + if math.IsNaN(c.BufferFraction) || !(c.BufferFraction >= 0 && c.BufferFraction < 1) { + return fmt.Errorf("buffer_fraction %g must be in [0, 1)", c.BufferFraction) + } + return nil +} + +// validateLadderRunLimits checks the per-run spend and action caps and the +// buffer utilization threshold. +func (c *LadderConfigDB) validateLadderRunLimits() error { + if v := c.MaxHourlyCommitPerRun; v != nil { + if math.IsNaN(*v) || math.IsInf(*v, 0) || !(*v > 0) { + return fmt.Errorf("max_hourly_commit_per_run %g must be a positive finite number when set (omit for no cap)", *v) + } + } + if c.MaxActionsPerRun <= 0 { + return fmt.Errorf("max_actions_per_run %d must be > 0", c.MaxActionsPerRun) + } + if c.MaxActionsPerRun > MaxLadderActionsPerRun { + return fmt.Errorf("max_actions_per_run %d exceeds ceiling %d", c.MaxActionsPerRun, MaxLadderActionsPerRun) + } + if math.IsNaN(c.BufferUtilizationThreshold) || !(c.BufferUtilizationThreshold > 0 && c.BufferUtilizationThreshold <= 100) { + return fmt.Errorf("buffer_utilization_threshold %g must be in (0, 100]", c.BufferUtilizationThreshold) + } + return nil +} + +// validateLadderRampSchedule parses the JSONB ramp_schedule payload and +// delegates structural validation to pkg/ladder.RampSchedule.Validate so +// the two validation paths stay in sync. +func (c *LadderConfigDB) validateLadderRampSchedule() error { + if len(c.RampSchedule) == 0 { + return fmt.Errorf("ramp_schedule must not be empty") + } + var rs ladder.RampSchedule + if err := json.Unmarshal(c.RampSchedule, &rs); err != nil { + return fmt.Errorf("ramp_schedule is not valid JSON: %w", err) + } + if err := rs.Validate(); err != nil { + return fmt.Errorf("ramp_schedule: %w", err) + } + return nil +} diff --git a/internal/config/validation_ladder_test.go b/internal/config/validation_ladder_test.go new file mode 100644 index 000000000..0a9cb6a06 --- /dev/null +++ b/internal/config/validation_ladder_test.go @@ -0,0 +1,174 @@ +package config + +import ( + "encoding/json" + "math" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// validLadderConfigDB returns a LadderConfigDB that passes Validate(). Each +// bounds test below clones this and mutates exactly one field so a failure +// pinpoints the single offending invariant. +func validLadderConfigDB() LadderConfigDB { + return LadderConfigDB{ + CloudAccountID: "11111111-1111-1111-1111-111111111111", + Provider: "aws", + Enabled: true, + Mode: "email_approval", + Cadence: "daily", + TargetCoverage: 100, + BufferFraction: 0.10, + BaselinePercentile: 5, + LookbackDays: 30, + BufferUtilizationThreshold: 90, + MaxActionsPerRun: 10, + MaxHourlyCommitPerRun: nil, // nil = no cap (valid) + RampSchedule: json.RawMessage(`{"steps":[{"after_days":0,"fraction":1.0}]}`), + } +} + +// TestLadderConfigDB_Validate_Bounds exercises every numeric bound of +// LadderConfigDB.Validate() at below-min / at-lower-bound / at-upper-bound / +// above-max / NaN (for float fields), asserting error vs nil per the documented +// ranges: +// +// target_coverage (0, 100] +// baseline_percentile (0, 50] +// buffer_fraction [0, 1) -- note: 0 is VALID ("no buffer") +// buffer_utilization_threshold (0, 100] +// lookback_days > 0 +// max_actions_per_run > 0 and <= MaxLadderActionsPerRun (50) +// +// Struct field order (func, string, bool) is chosen for govet fieldalignment; +// the keyed literals below keep the human-readable name/mutate/wantErr order. +func TestLadderConfigDB_Validate_Bounds(t *testing.T) { + cases := []struct { + mutate func(c *LadderConfigDB) + name string + wantErr bool + }{ + // baseline valid + {name: "valid baseline", mutate: func(c *LadderConfigDB) {}, wantErr: false}, + + // target_coverage (0, 100] + {name: "target_coverage 0 (below min)", mutate: func(c *LadderConfigDB) { c.TargetCoverage = 0 }, wantErr: true}, + {name: "target_coverage 0.01 (just above lower)", mutate: func(c *LadderConfigDB) { c.TargetCoverage = 0.01 }, wantErr: false}, + {name: "target_coverage 100 (upper bound)", mutate: func(c *LadderConfigDB) { c.TargetCoverage = 100 }, wantErr: false}, + {name: "target_coverage 100.01 (above max)", mutate: func(c *LadderConfigDB) { c.TargetCoverage = 100.01 }, wantErr: true}, + {name: "target_coverage NaN", mutate: func(c *LadderConfigDB) { c.TargetCoverage = math.NaN() }, wantErr: true}, + + // baseline_percentile (0, 50] + {name: "baseline_percentile 0 (below min)", mutate: func(c *LadderConfigDB) { c.BaselinePercentile = 0 }, wantErr: true}, + {name: "baseline_percentile 0.01 (just above lower)", mutate: func(c *LadderConfigDB) { c.BaselinePercentile = 0.01 }, wantErr: false}, + {name: "baseline_percentile 50 (upper bound)", mutate: func(c *LadderConfigDB) { c.BaselinePercentile = 50 }, wantErr: false}, + {name: "baseline_percentile 50.01 (above max)", mutate: func(c *LadderConfigDB) { c.BaselinePercentile = 50.01 }, wantErr: true}, + {name: "baseline_percentile NaN", mutate: func(c *LadderConfigDB) { c.BaselinePercentile = math.NaN() }, wantErr: true}, + + // buffer_fraction [0, 1) -- 0 is valid, 1 is excluded + {name: "buffer_fraction 0 (valid lower bound, no buffer)", mutate: func(c *LadderConfigDB) { c.BufferFraction = 0 }, wantErr: false}, + {name: "buffer_fraction -0.01 (below min)", mutate: func(c *LadderConfigDB) { c.BufferFraction = -0.01 }, wantErr: true}, + {name: "buffer_fraction 0.999 (just below upper)", mutate: func(c *LadderConfigDB) { c.BufferFraction = 0.999 }, wantErr: false}, + {name: "buffer_fraction 1.0 (upper bound excluded)", mutate: func(c *LadderConfigDB) { c.BufferFraction = 1.0 }, wantErr: true}, + {name: "buffer_fraction NaN", mutate: func(c *LadderConfigDB) { c.BufferFraction = math.NaN() }, wantErr: true}, + + // buffer_utilization_threshold (0, 100] + {name: "buffer_utilization_threshold 0 (below min)", mutate: func(c *LadderConfigDB) { c.BufferUtilizationThreshold = 0 }, wantErr: true}, + {name: "buffer_utilization_threshold 0.01 (just above lower)", mutate: func(c *LadderConfigDB) { c.BufferUtilizationThreshold = 0.01 }, wantErr: false}, + {name: "buffer_utilization_threshold 100 (upper bound)", mutate: func(c *LadderConfigDB) { c.BufferUtilizationThreshold = 100 }, wantErr: false}, + {name: "buffer_utilization_threshold 100.01 (above max)", mutate: func(c *LadderConfigDB) { c.BufferUtilizationThreshold = 100.01 }, wantErr: true}, + {name: "buffer_utilization_threshold NaN", mutate: func(c *LadderConfigDB) { c.BufferUtilizationThreshold = math.NaN() }, wantErr: true}, + + // lookback_days > 0 + {name: "lookback_days 0 (below min)", mutate: func(c *LadderConfigDB) { c.LookbackDays = 0 }, wantErr: true}, + {name: "lookback_days -1 (negative)", mutate: func(c *LadderConfigDB) { c.LookbackDays = -1 }, wantErr: true}, + {name: "lookback_days 1 (lower bound)", mutate: func(c *LadderConfigDB) { c.LookbackDays = 1 }, wantErr: false}, + + // max_actions_per_run > 0 and <= 50 + {name: "max_actions_per_run 0 (below min)", mutate: func(c *LadderConfigDB) { c.MaxActionsPerRun = 0 }, wantErr: true}, + {name: "max_actions_per_run 1 (lower bound)", mutate: func(c *LadderConfigDB) { c.MaxActionsPerRun = 1 }, wantErr: false}, + {name: "max_actions_per_run 50 (upper bound)", mutate: func(c *LadderConfigDB) { c.MaxActionsPerRun = MaxLadderActionsPerRun }, wantErr: false}, + {name: "max_actions_per_run 51 (above max)", mutate: func(c *LadderConfigDB) { c.MaxActionsPerRun = MaxLadderActionsPerRun + 1 }, wantErr: true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := validLadderConfigDB() + tc.mutate(&c) + err := c.Validate() + if tc.wantErr { + require.Error(t, err, "expected Validate() to reject case %q", tc.name) + } else { + require.NoError(t, err, "expected Validate() to accept case %q", tc.name) + } + }) + } +} + +// TestLadderConfigDB_Validate_MaxHourlyCommitPerRun covers the pointer-typed +// optional cap: nil = no cap (valid), a positive value is valid, and +// non-positive / NaN / Inf are rejected. +func TestLadderConfigDB_Validate_MaxHourlyCommitPerRun(t *testing.T) { + f := func(v float64) *float64 { return &v } + cases := []struct { + val *float64 + name string + wantErr bool + }{ + {name: "nil (no cap)", val: nil, wantErr: false}, + {name: "positive", val: f(12.5), wantErr: false}, + {name: "zero", val: f(0), wantErr: true}, + {name: "negative", val: f(-1), wantErr: true}, + {name: "NaN", val: f(math.NaN()), wantErr: true}, + {name: "positive Inf", val: f(math.Inf(1)), wantErr: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := validLadderConfigDB() + c.MaxHourlyCommitPerRun = tc.val + err := c.Validate() + if tc.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +// TestLadderConfigDB_Validate_EnumsAndProvider covers the delegated enum checks +// (mode/cadence via pkg/ladder Parse*) and the provider allow-list. +func TestLadderConfigDB_Validate_EnumsAndProvider(t *testing.T) { + cases := []struct { + mutate func(c *LadderConfigDB) + name string + wantErr bool + }{ + {name: "unknown provider", mutate: func(c *LadderConfigDB) { c.Provider = "digitalocean" }, wantErr: true}, + {name: "empty provider", mutate: func(c *LadderConfigDB) { c.Provider = "" }, wantErr: true}, + {name: "gcp provider", mutate: func(c *LadderConfigDB) { c.Provider = "gcp" }, wantErr: false}, + {name: "unknown mode", mutate: func(c *LadderConfigDB) { c.Mode = "yolo_approve" }, wantErr: true}, + {name: "auto_approve mode", mutate: func(c *LadderConfigDB) { c.Mode = "auto_approve" }, wantErr: false}, + {name: "unknown cadence", mutate: func(c *LadderConfigDB) { c.Cadence = "hourly" }, wantErr: true}, + {name: "weekly cadence", mutate: func(c *LadderConfigDB) { c.Cadence = "weekly" }, wantErr: false}, + {name: "empty ramp_schedule", mutate: func(c *LadderConfigDB) { c.RampSchedule = nil }, wantErr: true}, + {name: "malformed ramp_schedule JSON", mutate: func(c *LadderConfigDB) { c.RampSchedule = json.RawMessage(`not-json`) }, wantErr: true}, + {name: "ramp fractions not summing to 1", mutate: func(c *LadderConfigDB) { + c.RampSchedule = json.RawMessage(`{"steps":[{"after_days":0,"fraction":0.5}]}`) + }, wantErr: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := validLadderConfigDB() + tc.mutate(&c) + err := c.Validate() + if tc.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/internal/database/postgres/migrations/000079_ladder_configs.down.sql b/internal/database/postgres/migrations/000079_ladder_configs.down.sql new file mode 100644 index 000000000..803730ff8 --- /dev/null +++ b/internal/database/postgres/migrations/000079_ladder_configs.down.sql @@ -0,0 +1,8 @@ +-- Rollback migration 000079: remove ladder_configs table and the +-- laddering_enabled kill-switch column from global_config. +-- DROP TABLE CASCADE removes the trigger automatically. + +DROP TABLE IF EXISTS ladder_configs; + +ALTER TABLE global_config + DROP COLUMN IF EXISTS laddering_enabled; diff --git a/internal/database/postgres/migrations/000079_ladder_configs.up.sql b/internal/database/postgres/migrations/000079_ladder_configs.up.sql new file mode 100644 index 000000000..4720b7af0 --- /dev/null +++ b/internal/database/postgres/migrations/000079_ladder_configs.up.sql @@ -0,0 +1,58 @@ +-- Migration 000079: commitment-laddering per-scope configuration table +-- and global_config kill-switch column. +-- +-- Default-off: laddering_enabled = false on the singleton global_config row; +-- per-account rows default to enabled = false. No laddering behaviour +-- activates until an operator explicitly enables both the global kill-switch +-- and at least one per-account row. +-- +-- Idempotent: ADD COLUMN IF NOT EXISTS and CREATE TABLE IF NOT EXISTS +-- throughout, so re-running after a partial failure is safe. + +-- Kill-switch on the global_config singleton row. +ALTER TABLE global_config + ADD COLUMN IF NOT EXISTS laddering_enabled BOOLEAN NOT NULL DEFAULT false; + +-- Per-account ladder configuration. +-- One row per (cloud_account_id, provider) pair: UNIQUE enforces the constraint. +-- ON DELETE CASCADE removes the config when the cloud account is deleted. +CREATE TABLE IF NOT EXISTS ladder_configs ( + id UUID PRIMARY KEY DEFAULT uuid_generate_v4(), + cloud_account_id UUID NOT NULL + REFERENCES cloud_accounts(id) ON DELETE CASCADE, + provider TEXT NOT NULL, + enabled BOOLEAN NOT NULL DEFAULT false, + mode TEXT NOT NULL, -- email_approval | auto_approve + cadence TEXT NOT NULL, -- daily | weekly + target_coverage NUMERIC(5,2) NOT NULL, + buffer_fraction NUMERIC(5,4) NOT NULL, + baseline_percentile NUMERIC(5,2) NOT NULL, + lookback_days INTEGER NOT NULL, + buffer_utilization_threshold NUMERIC(5,2) NOT NULL, + max_hourly_commit_per_run NUMERIC(20,6), -- NULL = no cap + max_actions_per_run INTEGER NOT NULL, + ramp_schedule JSONB NOT NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + + UNIQUE(cloud_account_id, provider) +); + +CREATE INDEX IF NOT EXISTS idx_ladder_configs_account + ON ladder_configs(cloud_account_id); + +CREATE INDEX IF NOT EXISTS idx_ladder_configs_provider + ON ladder_configs(provider); + +-- Trigger: keep updated_at current on every UPDATE. +-- Guard with DO block so re-runs are idempotent. +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_trigger WHERE tgname = 'update_ladder_configs_updated_at' + ) THEN + CREATE TRIGGER update_ladder_configs_updated_at + BEFORE UPDATE ON ladder_configs + FOR EACH ROW EXECUTE FUNCTION update_updated_at_column(); + END IF; +END $$; diff --git a/internal/database/postgres/migrations/000080_ladder_runs.down.sql b/internal/database/postgres/migrations/000080_ladder_runs.down.sql new file mode 100644 index 000000000..a6bfbfc88 --- /dev/null +++ b/internal/database/postgres/migrations/000080_ladder_runs.down.sql @@ -0,0 +1,14 @@ +-- Rollback migration 000080: remove ladder_run_id columns from +-- ri_exchange_history and purchase_executions, then drop ladder_runs. +-- Order matters: remove the FK-holding columns before dropping the +-- table they reference. + +DROP INDEX IF EXISTS idx_ri_exchange_history_ladder_run; +ALTER TABLE ri_exchange_history + DROP COLUMN IF EXISTS ladder_run_id; + +DROP INDEX IF EXISTS idx_purchase_executions_ladder_run; +ALTER TABLE purchase_executions + DROP COLUMN IF EXISTS ladder_run_id; + +DROP TABLE IF EXISTS ladder_runs; diff --git a/internal/database/postgres/migrations/000080_ladder_runs.up.sql b/internal/database/postgres/migrations/000080_ladder_runs.up.sql new file mode 100644 index 000000000..4c4ac71a5 --- /dev/null +++ b/internal/database/postgres/migrations/000080_ladder_runs.up.sql @@ -0,0 +1,93 @@ +-- Migration 000080: ladder_runs immutable audit table. +-- +-- Each ladder engine invocation writes one row here before taking any +-- action. Status tracks the lifecycle: +-- planned -> awaiting_approval -> approved -> executing -> completed +-- or -> failed | cancelled | expired (terminal states) +-- +-- Monetary baseline columns are all NULLABLE. NULL means "not yet computed" +-- or "run failed before this could be measured" -- never coerced to $0 +-- (project rule: absent numbers are NULL/pointer, never 0). +-- +-- Also adds ladder_run_id FK columns to purchase_executions and +-- ri_exchange_history so every commitment touched by a ladder run can be +-- traced back to the run that triggered it. +-- +-- Idempotent: CREATE TABLE IF NOT EXISTS and ADD COLUMN IF NOT EXISTS +-- throughout. + +CREATE TABLE IF NOT EXISTS ladder_runs ( + id UUID PRIMARY KEY DEFAULT uuid_generate_v4(), + config_id UUID REFERENCES ladder_configs(id) ON DELETE SET NULL, + started_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + completed_at TIMESTAMPTZ, + status TEXT NOT NULL, -- planned|awaiting_approval|approved|executing|completed|failed|cancelled|expired + mode TEXT, + cadence TEXT, + + -- Monetary baseline snapshot; all NULLABLE (NULL != $0, see docstring above). + baseline_usd_hr NUMERIC(20,6), + target_usd_hr NUMERIC(20,6), + existing_usd_hr NUMERIC(20,6), + gap_usd_hr NUMERIC(20,6), + + plan JSONB NOT NULL DEFAULT '{}', + + -- Running totals. NOT NULL DEFAULT 0 because these are accumulators + -- initialised to 0 at row creation, not baseline measurements. + total_hourly_commit NUMERIC(20,6) NOT NULL DEFAULT 0, + total_upfront_cost NUMERIC(20,6) NOT NULL DEFAULT 0, + estimated_savings NUMERIC(20,6) NOT NULL DEFAULT 0, + + -- Stores the SHA-256 hex digest of the run approval token, never the raw + -- token. The raw token is sent only in the approval email link; the + -- email-approval PR (phase-3 PR-3) must hash-on-write and use a + -- constant-time compare against this column. (The pre-existing + -- purchase_executions / ri_exchange_history approval_token columns still + -- store raw tokens; hashing those is tracked as a separate follow-up.) + approval_token_hash TEXT, + approval_token_expires_at TIMESTAMPTZ, + approved_by TEXT, + cancelled_by TEXT, + fire_at TIMESTAMPTZ, + + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +); + +CREATE INDEX IF NOT EXISTS idx_ladder_runs_config_id + ON ladder_runs(config_id); + +CREATE INDEX IF NOT EXISTS idx_ladder_runs_status + ON ladder_runs(status); + +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_trigger WHERE tgname = 'update_ladder_runs_updated_at' + ) THEN + CREATE TRIGGER update_ladder_runs_updated_at + BEFORE UPDATE ON ladder_runs + FOR EACH ROW EXECUTE FUNCTION update_updated_at_column(); + END IF; +END $$; + +-- Link purchase_executions to the ladder run that triggered them. +-- Partial index: only ladder-initiated executions carry the FK so the +-- index stays narrow and the NULL-check is free. +ALTER TABLE purchase_executions + ADD COLUMN IF NOT EXISTS ladder_run_id UUID REFERENCES ladder_runs(id) ON DELETE SET NULL; + +CREATE INDEX IF NOT EXISTS idx_purchase_executions_ladder_run + ON purchase_executions(ladder_run_id) + WHERE ladder_run_id IS NOT NULL; + +-- Link ri_exchange_history to the ladder run that triggered them. +-- ri_exchange_history was introduced in migration 000001 and is confirmed +-- present on main (most recently altered in migration 000077). +ALTER TABLE ri_exchange_history + ADD COLUMN IF NOT EXISTS ladder_run_id UUID REFERENCES ladder_runs(id) ON DELETE SET NULL; + +CREATE INDEX IF NOT EXISTS idx_ri_exchange_history_ladder_run + ON ri_exchange_history(ladder_run_id) + WHERE ladder_run_id IS NOT NULL; diff --git a/internal/database/postgres/migrations/000081_ladder_tranches.down.sql b/internal/database/postgres/migrations/000081_ladder_tranches.down.sql new file mode 100644 index 000000000..e4040fb20 --- /dev/null +++ b/internal/database/postgres/migrations/000081_ladder_tranches.down.sql @@ -0,0 +1,7 @@ +-- Rollback migration 000081: drop the ladder_tranches table. +-- run_id and every index on the table (including idx_ladder_tranches_run_id) +-- live ON this table, so DROP TABLE removes them all; no separate DROP COLUMN +-- is needed. The run_id -> ladder_runs foreign key is on the table being +-- dropped, so there is no drop-ordering constraint here. + +DROP TABLE IF EXISTS ladder_tranches; diff --git a/internal/database/postgres/migrations/000081_ladder_tranches.up.sql b/internal/database/postgres/migrations/000081_ladder_tranches.up.sql new file mode 100644 index 000000000..749ceae48 --- /dev/null +++ b/internal/database/postgres/migrations/000081_ladder_tranches.up.sql @@ -0,0 +1,50 @@ +-- Migration 000081: ladder_tranches table. +-- +-- A tranche is a single timed purchase slice within a ladder run's ramp +-- schedule. The scheduler fires each tranche at its scheduled_date, creates +-- a purchase_executions row, and links execution_id back here once the +-- commitment is bought. +-- +-- run_id traces a tranche back to the ladder_runs row that created it. It is +-- nullable because a tranche may be planned before its run row exists in later +-- flows, and ON DELETE SET NULL keeps the tranche for audit if the run is +-- later removed (mirrors config_id's semantics). Type matches ladder_runs.id +-- (UUID, see migration 000080). +-- +-- execution_id references purchase_executions(execution_id) rather than +-- purchase_executions(id) because callers resolve executions by their +-- execution_id business key. The UNIQUE constraint on execution_id +-- (migration 000008) satisfies the FK referential requirement. +-- +-- Idempotent: CREATE TABLE IF NOT EXISTS throughout. + +CREATE TABLE IF NOT EXISTS ladder_tranches ( + id UUID PRIMARY KEY DEFAULT uuid_generate_v4(), + config_id UUID REFERENCES ladder_configs(id) ON DELETE SET NULL, + run_id UUID REFERENCES ladder_runs(id) ON DELETE SET NULL, + layer_type TEXT NOT NULL, + amount_usd_hr NUMERIC(20,6) NOT NULL, + term TEXT NOT NULL, + payment_option TEXT NOT NULL, + scheduled_date TIMESTAMPTZ NOT NULL, + status TEXT NOT NULL, -- scheduled|fired|completed|cancelled|failed + execution_id UUID REFERENCES purchase_executions(execution_id) ON DELETE SET NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +); + +CREATE INDEX IF NOT EXISTS idx_ladder_tranches_config_id + ON ladder_tranches(config_id); + +-- Partial index: only run-linked tranches carry the FK, so the index stays +-- narrow and traces tranches back to their originating run cheaply. +CREATE INDEX IF NOT EXISTS idx_ladder_tranches_run_id + ON ladder_tranches(run_id) + WHERE run_id IS NOT NULL; + +CREATE INDEX IF NOT EXISTS idx_ladder_tranches_status + ON ladder_tranches(status); + +-- Partial index for the scheduler sweep that fires due tranches. +CREATE INDEX IF NOT EXISTS idx_ladder_tranches_scheduled + ON ladder_tranches(scheduled_date) + WHERE status = 'scheduled'; diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index f9f6b6401..9339192ae 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -77,6 +77,39 @@ func (m *MockConfigStore) SaveGlobalConfig(ctx context.Context, cfg *config.Glob return args.Error(0) } +// UpdateGlobalConfigAtomic mocks the atomic read-modify-write. When an +// expectation is registered it dispatches through m.Called (tests can drive the +// apply closure via a Run callback). Otherwise it emulates the real locked +// read-modify-write by routing through the mocked GetGlobalConfig + apply + +// SaveGlobalConfig, so existing tests that seed those two continue to work. +func (m *MockConfigStore) UpdateGlobalConfigAtomic(ctx context.Context, apply func(*config.GlobalConfig) error) (*config.GlobalConfig, error) { + if isExpected(&m.Mock, "UpdateGlobalConfigAtomic") { + args := m.Called(ctx, apply) + if args.Get(0) == nil { + return nil, args.Error(1) + } + v, ok := args.Get(0).(*config.GlobalConfig) + if !ok { + panic(fmt.Sprintf("mock: expected *config.GlobalConfig, got %T", args.Get(0))) + } + return v, args.Error(1) + } + existing, err := m.GetGlobalConfig(ctx) + if err != nil { + return nil, err + } + if existing == nil { + existing = &config.GlobalConfig{} + } + if err := apply(existing); err != nil { + return nil, err + } + if err := m.SaveGlobalConfig(ctx, existing); err != nil { + return nil, err + } + return existing, nil +} + // GetServiceConfig mocks the GetServiceConfig operation func (m *MockConfigStore) GetServiceConfig(ctx context.Context, provider, service string) (*config.ServiceConfig, error) { args := m.Called(ctx, provider, service) @@ -1287,6 +1320,54 @@ func (m *MockConfigStore) StampRIExchangeApprovedBy(ctx context.Context, id stri return args.Error(0) } +// GetLadderConfigs mocks the GetLadderConfigs operation. +// Returns an empty slice when no expectation is registered so tests that do +// not exercise laddering keep working without mock setup. +func (m *MockConfigStore) GetLadderConfigs(ctx context.Context) ([]config.LadderConfigDB, error) { + if !isExpected(&m.Mock, "GetLadderConfigs") { + return []config.LadderConfigDB{}, nil + } + args := m.Called(ctx) + if args.Get(0) == nil { + return nil, args.Error(1) + } + v, ok := args.Get(0).([]config.LadderConfigDB) + if !ok { + panic(fmt.Sprintf("mock: expected []config.LadderConfigDB, got %T", args.Get(0))) + } + return v, args.Error(1) +} + +// GetLadderConfig mocks the GetLadderConfig operation. +// Returns (nil, nil) when no expectation is registered. +func (m *MockConfigStore) GetLadderConfig(ctx context.Context, cloudAccountID, provider string) (*config.LadderConfigDB, error) { + if !isExpected(&m.Mock, "GetLadderConfig") { + return nil, nil + } + args := m.Called(ctx, cloudAccountID, provider) + if args.Get(0) == nil { + return nil, args.Error(1) + } + v, ok := args.Get(0).(*config.LadderConfigDB) + if !ok { + panic(fmt.Sprintf("mock: expected *config.LadderConfigDB, got %T", args.Get(0))) + } + return v, args.Error(1) +} + +// UpsertLadderConfig mocks the UpsertLadderConfig operation. +func (m *MockConfigStore) UpsertLadderConfig(ctx context.Context, cfg *config.LadderConfigDB) (*config.LadderConfigDB, error) { + args := m.Called(ctx, cfg) + if args.Get(0) == nil { + return nil, args.Error(1) + } + v, ok := args.Get(0).(*config.LadderConfigDB) + if !ok { + panic(fmt.Sprintf("mock: expected *config.LadderConfigDB, got %T", args.Get(0))) + } + return v, args.Error(1) +} + // isExpected reports whether mock has any .On() expectation for method. func isExpected(mock *mock.Mock, method string) bool { for _, call := range mock.ExpectedCalls { diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index 5400f2d47..f1962e4c6 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -319,3 +319,20 @@ func (m *mockConfigStoreForHealth) GetPurchaseHistoryInFlight(_ context.Context) func (m *mockConfigStoreForHealth) GetScheduledExecutionsDue(_ context.Context) ([]config.PurchaseExecution, error) { return nil, nil } + +func (m *mockConfigStoreForHealth) GetLadderConfigs(_ context.Context) ([]config.LadderConfigDB, error) { + return nil, nil +} +func (m *mockConfigStoreForHealth) GetLadderConfig(_ context.Context, _, _ string) (*config.LadderConfigDB, error) { + return nil, nil +} +func (m *mockConfigStoreForHealth) UpsertLadderConfig(_ context.Context, cfg *config.LadderConfigDB) (*config.LadderConfigDB, error) { + return cfg, nil +} +func (m *mockConfigStoreForHealth) UpdateGlobalConfigAtomic(_ context.Context, apply func(*config.GlobalConfig) error) (*config.GlobalConfig, error) { + cfg := &config.GlobalConfig{} + if err := apply(cfg); err != nil { + return nil, err + } + return cfg, nil +} diff --git a/pkg/ladder/types.go b/pkg/ladder/types.go index 299d094c2..76e06733f 100644 --- a/pkg/ladder/types.go +++ b/pkg/ladder/types.go @@ -281,14 +281,21 @@ const ( ) // RampStep is a single tranche within a ramp schedule. +// +// The json tags are load-bearing: the frontend sends and the DB stores this +// shape as snake_case (`{"after_days":N,"fraction":F}`). Without the tags Go's +// case-insensitive matching bridges `fraction`->Fraction but NOT +// `after_days`->AfterDays (the underscore breaks the match), so AfterDays would +// silently decode to 0 and multi-step ramps would fail RampSchedule.Validate +// (ascending-AfterDays) or collapse to fire at day 0. type RampStep struct { // AfterDays is the number of days after the run starts at which this // tranche fires. Must be strictly greater than the previous step's // AfterDays (ascending order required by RampSchedule.Validate). - AfterDays int + AfterDays int `json:"after_days"` // Fraction is the share of the total target allocation committed by this // tranche. Must be in (0, 1]; fractions across all steps must sum to 1.0. - Fraction float64 + Fraction float64 `json:"fraction"` } // RampSchedule spreads commitment purchases across time-indexed tranches. @@ -299,7 +306,7 @@ type RampStep struct { // and pkg/ cannot import internal/. See internal/config/types.go for the // internal variant. type RampSchedule struct { - Steps []RampStep + Steps []RampStep `json:"steps"` } // Validate checks that the ramp schedule is well-formed: diff --git a/pkg/ladder/types_test.go b/pkg/ladder/types_test.go index 665c50fe3..9e3c6f3b1 100644 --- a/pkg/ladder/types_test.go +++ b/pkg/ladder/types_test.go @@ -1,6 +1,7 @@ package ladder import ( + "encoding/json" "math" "strings" "testing" @@ -8,6 +9,45 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" ) +// TestRampSchedule_JSONSnakeCaseMultiStep is the F1 regression test. The +// frontend sends and the DB stores this shape as snake_case; without json tags +// on RampStep/RampSchedule, Go's case-insensitive field matching binds +// "fraction"->Fraction but NOT "after_days"->AfterDays (the underscore breaks +// the match), so every AfterDays would decode to 0 and a multi-step ramp would +// fail Validate's strictly-ascending check (or, for a single nonzero step, fire +// at day 0). This fails against the pre-tag code. +func TestRampSchedule_JSONSnakeCaseMultiStep(t *testing.T) { + const wantJSON = `{"steps":[{"after_days":0,"fraction":0.5},{"after_days":30,"fraction":0.3},{"after_days":60,"fraction":0.2}]}` + + var rs RampSchedule + if err := json.Unmarshal([]byte(wantJSON), &rs); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(rs.Steps) != 3 { + t.Fatalf("got %d steps, want 3", len(rs.Steps)) + } + for i, want := range []int{0, 30, 60} { + if rs.Steps[i].AfterDays != want { + t.Errorf("step %d AfterDays = %d, want %d (after_days json tag not applied?)", i, rs.Steps[i].AfterDays, want) + } + } + if rs.Steps[0].Fraction != 0.5 { + t.Errorf("step 0 Fraction = %g, want 0.5", rs.Steps[0].Fraction) + } + if err := rs.Validate(); err != nil { + t.Errorf("Validate() on the multi-step ramp = %v, want nil", err) + } + + // Round-trip: marshal back to the exact snake_case DB/frontend contract. + out, err := json.Marshal(rs) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if string(out) != wantJSON { + t.Errorf("marshal round-trip = %s, want %s", out, wantJSON) + } +} + func TestLayerTypeValidate(t *testing.T) { t.Parallel() cases := []struct {