feat(settings): configurable EC2 RI OfferingClass (closes #694) - #847
Merged
Merged
Conversation
cristim
added
triaged
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds EC2 Reserved Instance offering-class selection across the admin UI, typed configuration, PostgreSQL persistence, validation, purchase execution, and AWS offering lookup. Convertible remains the default; standard is accepted as an alternative. ChangesOffering Class Feature
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant SettingsAPI
participant ConfigStore
participant PurchaseExecution
participant EC2Client
Admin->>SettingsAPI: save offering_class
SettingsAPI->>ConfigStore: persist GlobalConfig
PurchaseExecution->>ConfigStore: load GlobalConfig
ConfigStore-->>PurchaseExecution: OfferingClass
PurchaseExecution->>EC2Client: pass PurchaseOptions.OfferingClass
EC2Client->>EC2Client: resolve offering class
EC2Client-->>PurchaseExecution: DescribeReservedInstancesOfferings result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Member
Author
|
pr-iterate: CR appears to be blocked by org-level usage limits ( |
Member
Author
|
@coderabbitai review |
Contributor
Rate Limit Exceeded
|
Member
Author
|
Rebased on feat/multicloud-web-frontend. Resolved additive conflict in providers/aws/services/ec2/client_test.go (kept both HEAD's Name-tag tests and PR's OfferingClass tests). @coderabbitai review |
Contributor
|
🧠 Learnings used✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
cristim
added a commit
that referenced
this pull request
Jun 8, 2026
cristim
added a commit
that referenced
this pull request
Jun 8, 2026
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/database/postgres/migrations/000078_ec2_ri_offering_class.up.sql (1)
5-6: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueOptional: consider a CHECK constraint for defense-in-depth.
The column currently has no DB-level constraint on allowed values, relying entirely on app-layer validation (
validateOfferingClass). A directUPDATE global_config SET offering_class = 'foo'via psql would silently persist an invalid value that only surfaces as an error later at purchase time (perresolveOfferingClassType).🛡️ Optional CHECK constraint
ALTER TABLE global_config ADD COLUMN IF NOT EXISTS offering_class TEXT NOT NULL DEFAULT 'convertible'; + +ALTER TABLE global_config + ADD CONSTRAINT global_config_offering_class_check + CHECK (offering_class IN ('convertible', 'standard'));Skip if the team intends to keep the value set open-ended without a migration for future classes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/postgres/migrations/000078_ec2_ri_offering_class.up.sql` around lines 5 - 6, Add a DB-level CHECK constraint for global_config.offering_class in this migration so only the supported offering class values can be stored, instead of relying solely on validateOfferingClass and resolveOfferingClassType. Update the migration that alters global_config to include the constraint alongside the existing offering_class column definition, using the same allowed values enforced by the app.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/database/postgres/migrations/000078_ec2_ri_offering_class.up.sql`:
- Around line 5-6: Add a DB-level CHECK constraint for
global_config.offering_class in this migration so only the supported offering
class values can be stored, instead of relying solely on validateOfferingClass
and resolveOfferingClassType. Update the migration that alters global_config to
include the constraint alongside the existing offering_class column definition,
using the same allowed values enforced by the app.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b979982a-e00d-4265-bddd-753143d78382
📒 Files selected for processing (19)
frontend/src/__tests__/settings.test.tsfrontend/src/api/types.tsfrontend/src/index.htmlfrontend/src/settings.tsfrontend/src/types.tsinternal/config/store_postgres.gointernal/config/store_postgres_coverage_test.gointernal/config/store_postgres_pgxmock_test.gointernal/config/types.gointernal/config/validation.gointernal/config/validation_test.gointernal/database/postgres/migrations/000078_ec2_ri_offering_class.down.sqlinternal/database/postgres/migrations/000078_ec2_ri_offering_class.up.sqlinternal/purchase/execution.gointernal/purchase/execution_test.gointernal/purchase/money_path_regression_test.gopkg/common/types.goproviders/aws/services/ec2/client.goproviders/aws/services/ec2/client_test.go
💤 Files with no reviewable changes (2)
- providers/aws/services/ec2/client.go
- providers/aws/services/ec2/client_test.go
✅ Files skipped from review due to trivial changes (2)
- frontend/src/types.ts
- frontend/src/index.html
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/purchase/money_path_regression_test.go
- internal/config/validation_test.go
- internal/purchase/execution_test.go
- internal/config/types.go
- pkg/common/types.go
- frontend/src/settings.ts
- internal/config/validation.go
- frontend/src/api/types.ts
- frontend/src/tests/settings.test.ts
- internal/config/store_postgres.go
- internal/purchase/execution.go
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Add a GlobalConfig.OfferingClass field ("convertible" | "standard") that
controls which EC2 Reserved Instance offering class is purchased. Empty /
absent values default to "convertible" to preserve pre-694 behaviour.
Backend:
- pkg/common/types.go: OfferingClass field on PurchaseOptions
- internal/config/types.go: OfferingClass on GlobalConfig
- internal/config/store_postgres.go: read/write offering_class column ($20)
- internal/purchase/execution.go: load GlobalConfig and propagate
OfferingClass into PurchaseOptions before the fan-out
- providers/aws/services/ec2/client.go: resolveOfferingClassType() fails
loudly on unknown values (feedback_empty_string_vs_error.md); empty
string maps to convertible so the DB default is safe
- internal/database/postgres/migrations/000064_ec2_ri_offering_class: ADD
COLUMN offering_class TEXT NOT NULL DEFAULT 'convertible'
Frontend:
- frontend/src/index.html: fieldset with <select> (Convertible / Standard)
- frontend/src/settings.ts: load, save, and reset handlers
- frontend/src/api/types.ts + types.ts: offering_class optional field
Tests:
- TestResolveOfferingClassType: unknown values error, "" = convertible
- TestDescribeInputFromQuery_OfferingClass: empty / explicit convertible / standard
- All findOfferingID call sites updated to 4-arg signature
- store_postgres_pgxmock_test.go: +offering_class column in mock rows
- settings.test.ts: offering_class: 'convertible' in saveGlobalSettings
expected payload
…EC2 SDK call (refs #694) - Add validateOfferingClass() to GlobalConfig.Validate() so an invalid offering_class is rejected at PUT time with a clear error rather than silently persisting and only failing at purchase time. Accepts "" | "convertible" | "standard" (case-sensitive, matching resolveOfferingClassType). - Add ValidOfferingClasses exported var for reference/documentation. - Add six table-driven test cases in TestGlobalConfig_Validate covering empty (valid), both valid values, wrong-case "Convertible", "STANDARD", and an unknown value. - Add capturingMockEC2Client embedding MockEC2Client that records the last DescribeReservedInstancesOfferingsInput to enable SDK-level assertions. - Add TestFindOfferingID_OfferingClassReachesSDKCall: three subtests ("", "convertible", "standard") each calling findOfferingID and asserting that the captured OfferingClass on the outbound SDK call equals the expected types.OfferingClassType. This test fails if the wiring from offeringClassStr through resolveOfferingClassType to describeInputFromQuery regresses.
…cyclomatic complexity Validate had complexity 11 after adding the OfferingClass check in #694. Extract the CollectionSchedule + NotificationDaysBefore + GracePeriodDays checks into validateScheduleAndNotifications, keeping Validate under 10.
…sSDKCall Construct capturingMockEC2Client directly instead of copying *MockEC2Client to silence the go vet copylocks warning (MockEC2Client embeds mock.Mock which contains sync.Mutex).
… defaulting OfferingClass When GetGlobalConfig returns an error, the old code logged it and continued with an empty OfferingClass, which resolveOfferingClassType maps to "convertible". This caused a transient DB failure to silently buy the wrong (and more expensive, irreversible) RI class instead of the operator-configured "standard". Violates the no-silent-fallbacks-on-money-paths rule. Fix: change processPurchaseRecommendations to return (float64, float64, []string, error) and propagate a hard error on GetGlobalConfig failure. Both call sites (executeSingleAccount and executeForAccount) check the new error return and abort without making any cloud purchase. Regression test (HOLE 1): TestProcessPurchaseRecommendations_GlobalConfigError_FailsInsteadOfDefaulting confirms the function returns an error and never calls the provider factory when GetGlobalConfig fails. Verified red on pre-fix code, green after. Also add TestSaveGlobalConfig_OfferingClassBindsAt21 (HOLE 2): a pgxmock test against the real PostgresStore.SaveGlobalConfig verifying offering_class binds as the 21st positional arg. The hand-maintained testablePostgresStore in store_postgres_mock_test.go omits this field entirely, so no fast test previously guarded the 21-placeholder write query.
…ount below gocyclo 10 The gocyclo pre-commit hook (-over 10) failed: executeForAccount was at cyclomatic complexity 11. Extract the per-account status-resolution switch (partially_completed / failed / completed stamping, #642) and the committed gate (#1014) into a cohesive applyAccountOutcome helper, dropping the function below the threshold without a //nolint or a threshold bump. Behavior is unchanged: same status transitions, same error-note appending, same committed semantics (anyRecPurchased is now evaluated once and reused for the partial flag). The OfferingClass typed-enum validation and no-silent-fallback behavior are untouched.
Rebasing onto main added laddering_enabled as $21 in global_config INSERT, shifting offering_class from $21 to $22. Update the pgxmock and coverage tests to reflect the new column count and binding positions.
Resolve the three golangci-lint findings that --new-from-rev=origin/main attributes to the #694 OfferingClass changes: - errcheck: replace the silent `_ =` discard of SavePurchaseExecution on the processPurchaseRecommendations error path with the existing saveExecutionStatusBestEffort helper, which persists and logs the audit-save failure instead of dropping it. - gocritic unnamedResult: name the four results of processPurchaseRecommendations (its return set grew to include error); switch the trailing assignment from := to = accordingly. - misspell: pre-694 behaviour -> behavior in the ValidOfferingClasses doc. No behavior change to the purchase path; base-debt Lint/Security failures are pre-existing on main and out of scope.
Migration 000083 is now taken by ladder_execution_enabled (merged to main while this PR was in review). Next free slot after 000089_rename_cancelled_to_canceled is 000090.
…uard test Two issues introduced when rebasing onto main (which added ladder_execution_enabled in parallel): 1. saveExecutionStatusBestEffort helper was dropped during conflict resolution of the applyAccountOutcome refactor commit; restored from the pre-rebase branch. 2. TestSaveGlobalConfig_OfferingClassBindsAt22 expected 22 args; with ladder_execution_enabled now at $22, offering_class moved to $23. Renamed the test to TestSaveGlobalConfig_OfferingClassBindsAt23 and added the $22 AnyArg placeholder.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
30 tasks
cristim
added a commit
that referenced
this pull request
Sep 27, 2026
…290) (#804) * feat(purchases): in-app revocation within free-cancel window (closes #290) POST /api/purchases/{purchaseId}/revoke: Azure returns via armreservations (CalculateRefund + Return two-step), 7-day window. AWS/GCP return 422 and the History UI hides the button for those providers. - DB: migration 000057 adds revocation_window_closes_at, revoked_at, revoked_via, support_case_id columns to purchase_history - RBAC: revoke-own / revoke-any actions, revoke-own granted to all users by default; ownership is via account-access (history rows pre-date created_by_user_id) - Backend: fail-closed nil-auth guard, idempotency via revoked_at IS NULL, Azure CalculateRefund->Return with test-injectable client interfaces - Frontend: canRevokeCompletedRow gate (azure + window open + not yet revoked), Revoke button in History action cell, confirm dialog + toast - All existing mock stores updated with GetPurchaseHistoryByPurchaseID and MarkPurchaseRevoked; auth permission-count tests updated to 12 * fix(ci): extract provider dispatch and account check to reduce revoke complexity; regenerate permissions on PR #804 * fix(api/purchases): align revoke admin check with group-only authz (#907) Rebase onto feat/multicloud-web-frontend brought in #907 (group-membership- only authorization, no role field on Session). The revoke handler still gated admin via `session.Role == "admin"`, which no longer compiles since api.Session has no Role field. Replace with the same two-track pattern the sibling authorizeSessionCancel / authorizeSessionApprove already use: - Stateless admin API key short-circuits via `session.UserID == apiKeyAdminUserID` (no DB row exists to resolve permissions from). - Group-based admins fall through to HasPermissionAPI; the {admin, *} wildcard in DefaultAdminPermissions matches revoke-any:purchases there. Drop the dead `Role` field from the revoke test sessions; the admin test now pins the apiKeyAdminUserID short-circuit, and the existing RevokeAny test already covers the group-admin path via HasPermissionAPI. Also fold in the trailing pre-commit fixes that were red on the previous push: - gofmt: realign struct-field padding in TestRevokePurchase_AzureReturnClientError. - go mod tidy: promote armreservations from indirect to direct (the revoke handler imports it directly). Free-cancel window enforcement (AzureRevocationWindowDays + windowClosesAt check) and revoke-call idempotency (early return when record.RevokedAt is already set) are unchanged. Refs #290 * fix(api/purchases/revoke): fail-closed on nil account + return error on DB persist failure Two security fixes from CR findings: 1. checkRevokeOwnAccountAccess: return 403 when CloudAccountID is nil/empty instead of allowing the revoke. Without an account association, ownership cannot be verified for revoke-own callers. Add regression test for this fail-closed contract. 2. revokeAzurePurchase: return error when MarkPurchaseRevoked fails after a successful Azure return. The previous log-and-continue left the DB unmarked, breaking idempotency on retries. The error message notes that the refund was submitted so operators can investigate without re-issuing. * fix(purchases/revoke): populate revocation window at write path so the button works The Revoke button was shipped but dead: RevocationWindowClosesAt was never populated when a completed purchase was written, so the frontend gate canRevokeCompletedRow (which bails on a missing revocation_window_closes_at) hid the button on every real row. - Stamp RevocationWindowClosesAt in the real write path (purchase.savePurchaseHistory): Azure = Timestamp + 7 days (the free-cancel window), nil for AWS/GCP (out of Phase-1 scope), via a new shared config.RevocationWindowClosesAtFor helper + config.AzureRevocationWindowDays constant that is now the single source of truth for the window length. - Make the backend window check (revokeAzurePurchase) read RevocationWindowClosesAt as the source of truth, falling back to recomputing from Timestamp only for legacy rows written before the column was populated. - Reject an order-only ARM path (empty reservationID) in callAzureReturn rather than submitting an empty Return to Azure. - Tests: backend asserts savePurchaseHistory stamps the window for Azure and leaves it nil for AWS/GCP; handler asserts the stamped window drives the deny decision and that an empty reservationID is rejected; FE test asserts the Revoke button shows for a completed Azure row with a populated revocation_window_closes_at and is hidden without it (plus closed-window, already-revoked, non-Azure, and anonymous cases). * docs(auth/revoke): correct revoke-own doc to account-scope reality (#950) The ActionRevokeOwn doc comment claimed "Own" means created_by_user_id matches the session user, but checkRevokeOwnAccountAccess actually enforces ACCOUNT scope (GetAllowedAccountsAPI), because purchase_history rows pre-date created_by_user_id and have no reliable per-creator attribution. Fix the doc comments to describe the account-scope behavior as implemented; the authz model itself is unchanged. Whether revoke-own should instead be creator-scoped is a product decision tracked in issue #950, noted inline in both the auth constant doc and the handler check. * feat(purchases/revoke): Gmail-style pre-fire delay unifies revoke across providers Adds status=scheduled to purchase_executions so the approval step defers the cloud SDK call by purchase_delay_hours. A scheduled execution can be cancelled at $0 via the existing revoke endpoint before the scheduler fires. Two-tier revoke path: - status=scheduled: CancelExecutionAtomic (CAS) transitions to cancelled; no cloud API call, returns 200 with explicit "no cost incurred" message. - status=completed (existing): provider SDK call via purchase_history path. revokePurchase now tries GetExecutionByID first; falls through to the existing purchase_history lookup when no scheduled execution is found. authorizeSessionRevokeExecution mirrors authorizeSessionRevoke but uses CreatedByUserID (not CloudAccountID) for revoke-own scoping. FireScheduledDelayedPurchases (purchase.Manager) drives the scheduler tick: queries GetScheduledExecutionsDue, CAS-transitions scheduled->approved, stamps ApprovedBy="scheduler", calls executeAndFinalize. Registered as TaskFireScheduledPurchases in the Lambda task dispatcher. Also folds in three CodeRabbit findings from PR #804 review f9c66c1e: - Remove unused mockAuth in two AzureCalcRefundClientError/ReturnClientError tests - Add idempotent audit CHECK constraints to migration 000065 (revoked_via, support_case_id, revoked_at/revoked_via pair) - Frontend regression test: legacy blank-status Azure rows stay revocable - analytics/collector_test.go: hook-backed GetPurchaseHistoryByPurchaseID and MarkPurchaseRevoked mocks (no more hardcoded no-ops) * refactor(api/config): extract helpers to keep revoke+approve+email+scan under gocyclo limit revokePurchase -> loadAndRevokePurchaseHistory (handler_purchases_revoke.go) approvePurchase -> approveViaToken (handler_purchases.go) sendPurchaseScheduledEmail -> buildScheduledEmailData (handler_purchases.go) scanExecutionRows -> applyNullTimesToExecution (store_postgres.go) Also applies gofmt alignment fix to internal/analytics/collector_test.go. * refactor(test/mocks): consolidate MockConfigStore into shared internal/mocks (#804 cleanup) Three per-package MockConfigStore definitions (internal/api, internal/purchase, internal/scheduler) were duplicating ~450 LOC each every time a new StoreInterface method was added. Replace all three with a type alias pointing at internal/mocks.MockConfigStore. Changes to internal/mocks/stores.go: - Add Fn-override fields imported from the api and purchase local mocks (GetCloudAccountFn, GetPurchasePlanFn, SetPlanAccountsFn, SavePurchaseExecutionFn, GetPlanAccountsFn, and 6 others) so callers that used those fields keep working without changes. - Add isExpected guards to recommendation-cache and RI-utilization-cache methods and to CancelExecutionAtomic so existing tests that call these without explicit On() expectations don't panic (matches the "opt-in" pattern the per-package mocks used via hasRecExpectation / hasExpectation). - Add 8 interface methods that were missing from the shared mock but present in all per-package variants: GetExecutionsByStatuses, GetPlannedExecutions, GetStaleApprovedExecutions, ListStuckExecutions, GetScheduledExecutionsDue, MarkCollectionStarted, ClearCollectionStarted, StampRIExchangeApprovedBy. - Add compile-time check: var _ config.StoreInterface = (*MockConfigStore)(nil). - Promote GetGlobalConfig and GetPurchasePlan to return sensible defaults when no expectation is registered (matches the api-package behaviour these tests relied on). Per-package files reduced to a single type alias line each. The scheduler test also had stray suppression/Tx method stubs added in a later commit; those are removed because the methods already live on the shared mock. Intentionally left local (incompatible shape or semantics): - internal/analytics/collector_test.go: mockConfigStore (lowercase) -- hook-field only pattern with no testify embedding; analytics-specific subset; different name. - internal/server/test_helpers_test.go: mockConfigStoreForHealth -- all-zero-value stubs for health check tests; distinct type name; no testify embedding. - internal/server/handler_ri_exchange_test.go: mockConfigStoreForExchange -- test- specific struct overriding a handful of methods. - internal/server/handler_coverage_test.go: mockConfigStoreForExchange{Complete, Fail,Stale} -- per-scenario stubs with distinct type names. Net LOC: +274 insertions / -1601 deletions (-1327 net across 4 files). * fix(api/purchases/revoke): use status='scheduled' CAS so pre-fire revoke isn't dead The pre-fire delay revoke path called CancelExecutionAtomic, whose SQL guard is `WHERE status IN ('pending','notified')`. A status='scheduled' row never matches, so the CAS returned zero rows on the happy path -- the handler then surfaced 410 "revocation window has closed" even when the window was wide open. The user would pay the cloud charge AND see a "cancelled" attempt in the UI -- the worst possible outcome. The bug was hidden by mocks defaulting CancelExecutionAtomic to (true,"cancelled",nil), so every test in the scheduled-revoke suite was green against the wrong SQL. No pgxmock test exercised the WHERE clause. Fix: introduce CancelScheduledExecutionAtomic with the correct `WHERE status = 'scheduled'` guard and switch the handler to it. The two CAS variants are kept distinct on purpose -- the scheduled-revoke flow surfaces 410 ("scheduler already fired") on race-loss, while the pre-purchase cancel flow surfaces 409 ("not pending"). Sharing one method would conflate the two race outcomes. Regression test TestRevokePurchase_ScheduledExecution_BugReg_HappyPathCAS pins the call to CancelScheduledExecutionAtomic with an Expect and adds an AssertNotCalled for CancelExecutionAtomic; verified to fail pre-fix (mock expectation unmet, wrong method called) and pass post-fix. Touches: internal/config/{store_postgres,interfaces}.go -- add method + comments internal/api/handler_purchases_revoke{,_test}.go -- switch call site + reg test internal/mocks/stores.go -- mock the new method (default happy path) internal/server/test_helpers_test.go, internal/analytics/collector_test.go -- satisfy StoreInterface * fix(frontend/history): gate Revoke button on revoke-{any,own} permission canRevokeCompletedRow only checked getCurrentUser() truthiness, so the inline Revoke button rendered for every signed-in user regardless of the revoke-any / revoke-own grant. The backend correctly 403s, but the UX-vs-RBAC drift is exactly what PR #995 caught for approve / delete on the same page. The peer predicates (canCancelPendingRow, canApprovePendingRow, canRetryFailedRow) already check canAccess; canRevokeCompletedRow now does too. Verbs match the backend handler one-to-one: - admin or revoke-any:purchases -> always allowed - revoke-own:purchases -> allowed (account-scope enforced server-side) - anything else -> hidden Adds revoke-own / revoke-any to the closed Action union in permissions.ts so a future drift becomes a compile error at the canAccess call site. Adds a regression test that mocks getCurrentUser with an effectivePermissions set lacking revoke-* and asserts the button is hidden; verified to fail without the canAccess gate and pass with it. Also corrects the existing ADMIN_USER fixture to use the real ADMINISTRATORS_GROUP_ID GUID -- the prior 'administrators' label was inert because the new canAccess fallback drives off isAdmin() which checks GUID membership. * test(frontend/permissions): update USER_PERMS expected set for revoke-own PR #804 added 'revoke-own:purchases' to USER_PERMS in permissions.generated.ts but missed updating the user-role expected list in __tests__/permissions.test.ts. The test asserts perms.size matches expected.length, so the missing entry surfaced as "Expected 11, received 12" after the addition. This is the same scope as the parent permissions add -- not a separate permission grant, just the test parity update PR #804 should have included alongside the original add. * fix(server): table-drive ParseScheduledEvent + cover scheduled-fire sweep The fire_scheduled_purchases case pushed ParseScheduledEvent's cyclomatic complexity to 11, tripping the gocyclo<=10 pre-commit hook and leaving the PR UNSTABLE. Replace the action switch with a package-level lookup table so the complexity no longer grows with the task list; adding a task type stays a one-line change. Also close the test gap on the Gmail-style pre-fire delay scheduler sweep: FireScheduledDelayedPurchases / fireOneDue had no unit coverage of their result accounting or CAS-race classification (only the dispatch wiring was mocked). Add scheduled_fire_test.go mirroring reaper_test.go: - no-due-rows and list-error paths - CAS lost to a concurrent revoke -> RaceLost, not Errored, and no SDK fire (the safety property that prevents double-charging a revoked purchase) - row-vanished (ErrNotFound) -> RaceLost - hard DB error on the CAS -> Errored (per-row isolation, sweep still succeeds) Each race/error test fails if the classification regresses (verified by flipping fireOneDue's return). Add the fire_scheduled_purchases case to the ParseScheduledEvent table test so the new action is positively asserted. * fix(migrations): renumber 000068 -> 000070 to deconflict (refs #290) PR #808 keeps 000068 and PR #847 takes 000069; bump this branch's purchase_history_revocation migration to 000070 to avoid conflicts on feat/multicloud-web-frontend. * fix(scheduler): wire FireScheduledDelayedPurchases tick (CRITICAL: pre-fire delay branch was non-functional) - Add testify-based FireScheduledDelayedPurchases to scheduler's MockPurchaseManager so it records calls and satisfies mock.AssertExpectations. - Add TestSchedulerManagerInterface_FireScheduledDelayedPurchasesWired: compile-time guard that ManagerInterface exposes the method + call-recording smoke test. - Add TestFireScheduledDelayedPurchases_EndToEnd in scheduled_fire_test.go: skipped placeholder (with documented skip reason + issue ref) for the full provider-stub e2e once #1005 4-eyes lands. - Add TestFireScheduledDelayedPurchases_DelayPathNotSilentNoOp: compile-time guard that FireScheduledDelayedPurchases exists on Manager. The server/handler_test.go "fire_scheduled_purchases success" and "fire_scheduled_purchases propagates error" cases cover the full dispatch chain (ScheduledTaskType -> handleFireScheduledPurchases -> Purchase.FireScheduledDelayedPurchases). * fix(api/purchases): CAS-guard scheduleApprovedExecution to prevent silent revoke loss Replace the blind SavePurchaseExecution write in scheduleApprovedExecution with a two-step CAS pattern: 1. TransitionExecutionStatus(pending|notified -> scheduled) -- atomic CAS. 2. Stamp ScheduledExecutionAt + ApprovedBy on the returned post-CAS row. 3. SavePurchaseExecution to persist the stamps. Before this fix a concurrent Cancel that landed between the approve handler's SELECT and its SavePurchaseExecution would be silently overwritten: the cancelled row would become status="scheduled" and eventually fire the cloud SDK call the user explicitly revoked. The CAS ensures the write succeeds only when the row is still in pending or notified; a concurrent cancel causes ErrExecutionNotInExpectedStatus which surfaces as a clear error to the caller. Tests added: - TestHandler_scheduleApprovedExecution_CASGuardsConcurrentCancel: injects a concurrent-cancel error and asserts SavePurchaseExecution is never called. - TestHandler_scheduleApprovedExecution_HappyPath: normal flow, asserts ScheduledExecutionAt is stamped on the transitioned row. * feat(purchases/revoke): two-step quote-then-confirm + persist refund amount for audit Addresses adversarial-review Finding #4: - Add GET /api/purchases/revoke/calculate/{id} endpoint (calculateAzureRevoke) that calls CalculateRefund and returns the quoted refund amount and currency; no state mutation, used by the frontend confirmation modal. - Thread expectedRefundAmount through dispatchProviderRevoke -> revokeAzurePurchase -> callAzureReturn; on POST /revoke the client sends the amount it consented to and callAzureReturn re-runs CalculateRefund, rejecting with 422 when the new quote diverges by more than revokeQuoteEpsilon (0.01) to close the TOCTOU window between user confirmation and actual Return call. - Persist calc_refund_amount / calc_refund_currency via MarkPurchaseRevoked so the audit row captures the quoted values even if the actual refund differs later. - Migration 000071 adds the two new nullable columns and a consistency CHECK constraint (currency must be non-empty when amount is present). - New tests: TestCallAzureReturn_TOCTOUDivergenceRejectedWith422, TestCallAzureReturn_TOCTOUWithinEpsilonSucceeds, TestCallAzureReturn_AuditRowPopulatedWithQuote. * fix(purchases/revoke): partial-success reconciliation; never retry a refund that already succeeded Addresses adversarial-review Finding #6: - Migration 000072 adds revocation_in_flight BOOLEAN NOT NULL DEFAULT false to purchase_history plus a partial index on rows where the flag is true. - callAzureReturn flips revocation_in_flight=true via FlipPurchaseRevocationInFlight immediately before the Azure Return API call so the row is visible to the finalize sweep if the subsequent MarkPurchaseRevoked DB write fails. - MarkPurchaseRevoked is retried up to 3 times with 1s/3s/9s backoff after Azure Return succeeds; if all retries fail, the endpoint returns a revokeReconcilePendingResult (HTTP 207 body with code=RECONCILE_PENDING, azure_returned=true) so the frontend shows a non-retryable toast rather than prompting the user to retry a refund that Azure already issued. - loadAndRevokePurchaseHistory detects a row with revocation_in_flight=true and revoked_at=nil and immediately returns 207 RECONCILE_PENDING to prevent any duplicate Azure Return call on retry. - purchase.Manager.FinalizeInFlightRevocations sweeps rows matching GetPurchaseHistoryInFlight and retries MarkPurchaseRevoked with 2s/6s backoff per row; the finalize_revocations scheduled task wires this sweep into the Lambda event handler. - New tests: TestCallAzureReturn_MarkPurchaseRevokedFailAllRetries, TestLoadAndRevokePurchaseHistory_RevocationInFlightReturns207, finalize_revocations success and error dispatch tests. * fix(purchases/revoke): typed Azure error classification (no more substring match on err.Error()) Addresses adversarial-review Finding #7: Replace the string-based isAzureClientError implementation with typed error inspection using errors.As(err, &*azcore.ResponseError). The substring-match approach had two failure modes: - False positives: any error whose .Error() string contains "400" (e.g. a network timeout "timeout after 400ms") would be misclassified as a client error, hiding transient infra problems from the operator. - False negatives: Azure refund-policy errors with HTTP codes not in the literal set (e.g. 403, 405) would be escalated as 500. The typed approach classifies exactly the HTTP status codes Azure uses for policy violations and bad requests (400, 403, 404, 405, 409, 422); all other errors (transport errors, 5xx, plain errors) correctly classify as server-side. Updated TestRevokePurchase_AzureCalcRefundClientError to inject a real *azcore.ResponseError{StatusCode: 400} instead of errors.New("400: ..."). New tests: TestIsAzureClientError_SubstringFalsePositive, TestIsAzureClientError_TypedResponseError (covers 4xx client + 5xx server). * fix(purchases/revoke): 1h safety margin on local window + clean 422 on Azure window-edge rejection Addresses adversarial-review Finding #3: Add azureRefundSafetyMargin = 1h so the in-app revoke button disappears and revoke requests are rejected 1h before Azure's hard 7-day deadline. This eliminates the tail of RefundPolicyViolated failures caused by clock skew between CUDly's clock and Azure's at the window boundary. The safety margin is applied only in the local pre-flight check. The value stored in purchase_history.revocation_window_closes_at remains the unmodified Azure deadline so operators can see the true expiry. Additionally, detect RefundPolicyViolated errors from the Return API via the new isAzureWindowEdgeError helper (typed errors.As on *azcore.ResponseError, checking ErrorCode == "RefundPolicyViolated") and map them to a clean 422 with "window has closed" message rather than the generic "Azure refund rejected" 400 path. This handles the race where our safety-margin check passes but Azure's clock disagrees mid-flight. New tests: - TestRevokePurchase_AzureWithinSafetyMarginRejected: purchase 6d23h30m ago (30min before edge) rejected locally despite Azure deadline not yet passed. - TestRevokePurchase_AzureJustOutsideSafetyMarginAllowed: purchase 6d22h30m ago (90min before edge) accepted. - TestIsAzureWindowEdgeError: table test covering RefundPolicyViolated, other error codes, nil, and plain-error false-positive. - TestCallAzureReturn_RefundPolicyViolatedReturns422WindowEdge: Return API returning RefundPolicyViolated surfaces as 422, not 400. * fix(migrations): allow support-case revoke to record in-flight state (case filed, awaiting AWS) Addresses adversarial-review Finding #5: The original purchase_history_revoked_pair_chk required revoked_at and revoked_via to be set or unset together. This is too strict for the AWS support-case revocation path (issue #291 wave-2): when a case is filed, revoked_via='support-case' is recorded immediately for the audit trail, but revoked_at stays NULL until AWS confirms the refund. The pair check fires as a constraint violation in that in-flight state. Migration 000073 drops the pair check and simultaneously tightens the support-case companion check: Old: CHECK (support_case_id IS NULL OR revoked_via = 'support-case') (prevents support_case_id on non-support-case rows only) New: CHECK (revoked_via != 'support-case' OR support_case_id IS NOT NULL) (requires support_case_id whenever revoked_via = 'support-case') The meaningful invariant (no dangling revoked_at without a known provider path) is preserved by the existing purchase_history_revoked_via_chk which constrains revoked_via to ('direct-api', 'support-case'). * test(purchases/revoke): DST-crossing window math + 4-eyes approval placeholder Addresses adversarial-review Finding #9: TestRevocationWindowClosesAtFor_DSTCrossing: verifies that AddDate(0,0,7) (calendar arithmetic) produces the correct 7-day window across a DST transition. The test uses the 2024 US spring-forward on March 10 (02:00 EST -> 03:00 EDT): a purchase at 01:30 must close at 01:30 seven days later, not at 00:30 as a naive Add(168*time.Hour) would produce. This pins the correct behaviour and documents why a fixed-duration approach would be wrong. TestRevokePurchase_FourEyesApproval: skipped placeholder for the revoke 4-eyes approval gate tracked in issue #1005. The skip keeps the suite green while the feature is in flight and serves as a reminder to fill in the implementation when #1005 lands. * fix(api/purchases): map scheduleApprovedExecution CAS race to 409 (not 500) When a concurrent Cancel flips the execution away from schedulable status between the approve flow check and the CAS write, approveWithDelay now returns 409 instead of 500. ErrExecutionNotInExpectedStatus from TransitionExecutionStatus is the discriminator; any other error continues to surface as 500. * fix(api/purchases/revoke): drop pre-check, distinguish GetExecutionByID errors Two related fixes on the same line (revokePurchase GetExecutionByID branch): Finding B: Remove the racy status=="scheduled" pre-check. The old code read the status from the DB and then only dispatched to revokeScheduledExecution when it was "scheduled", but a concurrent writer could flip the status between the read and the CancelScheduledExecutionAtomic call. Drop the pre-check and let CancelScheduledExecutionAtomic's WHERE status='scheduled' CAS decide; a lost CAS returns 410 as expected. Finding C: Distinguish a genuine DB error (non-nil execErr) from a missing row (nil, nil). Before the fix, any non-nil execErr was folded into the "execErr == nil && ..." condition and silently swallowed, falling through to the history lookup. Now a non-nil execErr surfaces immediately as a wrapped error (500). * fix(api/purchases/revoke): clear revocation_in_flight on Azure error paths (transient retryable) When the Azure Return call fails (window-edge, client-error, or transient), the revocation_in_flight flag was left stuck at true. The finalize_revocations sweep would then treat the row as "Azure succeeded, DB write pending" and retry MarkPurchaseRevoked unnecessarily, potentially marking a purchase as revoked when it was never actually returned. Fix: call ClearRevocationInFlight (new store method) on all Azure Return error paths so the row reverts to its original status. On success the flag stays true for the sweep to handle (existing wave-1 behaviour unchanged). Adds ClearRevocationInFlight to StoreInterface, PostgresStore, and MockConfigStore. * fix(frontend/history): expose Revoke button for status='scheduled' rows + regression test Three related changes to surface the Revoke button on Gmail-style pre-fire delayed executions (status='scheduled') in the History UI: 1. Add "scheduled" to historyExecutionStatuses so these rows appear in the /api/history response at all (they were previously invisible). 2. Populate RevocationWindowClosesAt from ScheduledExecutionAt for scheduled rows in annotateHistoryRowByStatus so the frontend window check (revocation_window_closes_at in the future) works without a new field. 3. Update canRevokeCompletedRow to accept status==="scheduled" in addition to "completed" and "" (legacy blank), so the Revoke button renders. Adds regression test: a scheduled Azure row with a future revocation window must show the Revoke button (Findings E + G, second-wave CR). * fix(test/purchases/revoke): fix AssertNotCalled placement + isolate parallel test backoff state Finding F-1: Move AssertNotCalled(t, "CancelExecutionAtomic") to after the handler call in TestRevokePurchase_ScheduledExecution_BugReg_HappyPathCAS. The assertion was placed before h.revokePurchase(), where it trivially passes regardless of what the handler does. Finding F-2: Drop t.Parallel() from TestCallAzureReturn_MarkPurchaseRevokedFailAllRetries. The test mutates the package-global revokeMarkRetryBackoffs slice; running it in parallel with other tests that read the same variable is a data race. The test already uses t.Cleanup to restore the original value, which is sufficient when run sequentially. * fix(test/history-revoke): add missing mock entries for escapeHtmlAttr and getAmortizeUpfront The state mock lacked getAmortizeUpfront/setAmortizeUpfront/subscribeAmortizeUpfront and the utils mock lacked escapeHtmlAttr/amortizedMonthly. Both are called inside renderHistoryList; the missing entries caused a TypeError that loadHistory's try/catch swallowed, silently producing an empty history-list and hiding the Revoke button for all three "shows Revoke" cases. * fix(purchases/revoke): let the CAS decide scheduled cancellability (#290) Remove the early window-expiry 410 in revokeScheduledExecution. A row still in status="scheduled" has not been transitioned by the scheduler, so the cloud SDK call has not fired regardless of how far scheduled_execution_at is in the past (scheduler lag / backpressure). Returning 410 purely on a past timestamp broke free-cancel during lag even though CancelScheduledExecutionAtomic could still cancel the row before any cloud call. Let the CAS be the sole arbiter: it returns cancelled=false (410) only when the row has actually moved out of "scheduled". Updates the former WindowExpired test to assert the new contract (a past-timestamp scheduled row is cancelled for free via the CAS), and refreshes two stale "window-check" comments. Closes the last open CodeRabbit thread on PR #804. * fix(email/revoke): require recipient for scheduled-delay email; tidy tests (#290) Address the second CodeRabbit review pass on PR #804: - Security: SendPurchaseScheduledNotification (SES Sender) no longer falls back to the broadcast SendNotification path when RecipientEmail is empty. That email embeds a live, execution-scoped revoke link, so broadcasting it leaked an action link to every alert subscriber and broke the ownership/RBAC model around revocation. It now returns ErrNoRecipient, matching SendScheduledPurchaseNotification and the SMTP sender. Adds a regression test asserting ErrNoRecipient on empty recipient. - Test: rename TestRevokePurchase_AzureJustOutsideSafetyMarginAllowed -> TestCallAzureReturn_JustOutsideSafetyMargin and correct its docstring; it drives callAzureReturn directly and never exercised the local 1h safety-margin gate (which lives in dispatchProviderRevoke). The reject side of that gate stays covered end-to-end via TestRevokePurchase_AzureWithinSafetyMarginRejected. - Build: implement the ClearRevocationInFlight StoreInterface method on the standalone analytics and server test mocks (mockConfigStore, mockConfigStoreForExchange, mockConfigStoreForHealth) so those test packages compile after the interface gained the method. * refactor(purchases): reduce cyclomatic complexity below the pre-commit gate (#290) The gocyclo pre-commit hook (threshold 10) failed CI on four functions the #290 feature work grew past the limit. Split each into focused helpers with no behavior change: - calculateAzureRevoke (21): extract validateAzureRevokeRequest (auth + load + authorize + window/ID validation, itself split into azureRevokeWindowAndIDs) and extractAzureRefundQuote. - callAzureReturn (21): extract azureCalculateRefund (CalculateRefund + parse), handleAzureReturnError (clear in-flight + status mapping), and persistAzureRevocation (MarkPurchaseRevoked retry + 207 result). - annotateHistoryRowByStatus (12): move the in-flight / audit-gap cases into annotateInFlightOrAuditGapRow. - dispatchTask (11): switch to a map-based dispatch. gocyclo now reports nothing over 10; full internal test suite green (except the pre-existing CSRF flake that also fails on base).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GlobalConfig.OfferingClasssetting ("convertible" | "standard") that controls which EC2 Reserved Instance class is purchased; empty / absent values default to "convertible" preserving pre-694 behaviouroffering_class TEXT NOT NULL DEFAULT 'convertible'toglobal_config<select>values hardcoded in HTML so no innerHTML XSS surfaceresolveOfferingClassTypereturns an explicit error) perfeedback_empty_string_vs_error.mdTest plan
go build ./...cleango test github.com/LeanerCloud/CUDly/providers/aws/services/ec2/... github.com/LeanerCloud/CUDly/internal/config/...-- 601 passednpx tsc --noEmit-- no errorsnpx jest-- 2142 passed, 0 failedresolveOfferingClassType("")returnsOfferingClassTypeConvertible, nilresolveOfferingClassType("unknown")returns"", errorsaveGlobalSettingstest updated to assertoffering_class: 'convertible'in payloadSummary by CodeRabbit
New Features
Bug Fixes
Tests