From c88521b9f0cf6c2100e660a443a442adb6afbf17 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 27 Oct 2026 23:22:21 +0000 Subject: [PATCH 1/5] fix(config): delete only settled executions from CleanupOldExecutions' expiry branch The expires_at branch deleted a row in any status the plan health score does not count, so an approved or running row stranded by a dead executor was deleted once its TTL lapsed, before GetStaleApprovedExecutions or ListStuckExecutions could resolve it. Gate the branch on the allowlist cmd/cleanup-lambda already uses since #1173: status in TerminalExecutionStatuses and not in HealthScoredExecutionStatuses. Pending and notified rows now wait for ExpireExecutionAtomic to move them to expired, which the branch then reaps. Refs #120 Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres.go | 40 +++++--- ...store_postgres_cleanup_integration_test.go | 99 +++++++++++++++++++ internal/config/store_postgres_db_test.go | 11 ++- .../config/store_postgres_pgxmock_test.go | 37 ++++--- ...e_postgres_writer_race_integration_test.go | 6 +- 5 files changed, 168 insertions(+), 25 deletions(-) create mode 100644 internal/config/store_postgres_cleanup_integration_test.go diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index c4d9767b6..43677bb78 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -2316,13 +2316,26 @@ func (s *PostgresStore) GetScheduledExecutionsDue(ctx context.Context) ([]Purcha // already assumed. // // 3. Expired-row cleanup: `expires_at IS NOT NULL AND expires_at < NOW() - -// retention`, for any status the health score does not count. This is -// the row's own TTL column (SavePurchaseExecution writes it from +// retention`, for a settled row the health score does not count. This +// is the row's own TTL column (SavePurchaseExecution writes it from // PurchaseExecution.TTL), NOT the approval-token deadline -- that is a // separate column, `approval_token_expires_at`, added by migration // -// 000051. A row whose TTL lapsed longer than `retention` ago is dead, so -// without this branch it would accumulate indefinitely. +// 000051. A settled row whose TTL lapsed longer than `retention` ago is +// dead, so without this branch it would accumulate indefinitely. +// +// The status gate is an ALLOWLIST: status = ANY(TerminalExecutionStatuses) +// AND status <> ALL(HealthScoredExecutionStatuses), the same predicate +// cmd/cleanup-lambda's expiredExecutionsWhere uses (#998, #1173). A lapsed +// TTL says nothing about whether an executor, scheduler or reaper still +// needs the row: `expires_at` is set at insert and never rewritten, so an +// `approved` or `running` row stranded by a dead executor (the rows +// GetStaleApprovedExecutions and ListStuckExecutions exist to find) used +// to be deleted here before the reaper could resolve it (#120). Pending +// and notified rows are kept too: ExpireExecutionAtomic moves a lapsed +// approval to `expired`, which this branch then reaps. A status added to +// the schema later is kept until someone classifies it in +// TerminalExecutionStatuses, the fail-closed direction. // // Every status in HealthScoredExecutionStatuses is excluded here, which // is the whole reason that slice is exported rather than spelled out @@ -2365,11 +2378,15 @@ func (s *PostgresStore) CleanupOldExecutions(ctx context.Context, retentionDays // follow-up to #1277). #1278 will drop 'cancelled' once all rows are // normalized. // - // Branch 3's exclusion is parameterized from - // HealthScoredExecutionStatuses instead of repeating a literal list, so - // a status added to the plan health score extends this exclusion - // automatically and the two cannot drift. `status` is NOT NULL - // (migration 000001), so `<> ALL` is not exposed to three-valued logic. + // Branch 3's allowlist and exclusion are parameterized from + // TerminalExecutionStatuses and HealthScoredExecutionStatuses instead of + // repeating literal lists, so this branch, cmd/cleanup-lambda and the + // plan health score cannot drift. `status` is NOT NULL (migration + // 000001), so neither array test is exposed to three-valued logic. + // + // Under READ COMMITTED a row another transaction is updating is + // rechecked against the whole predicate once that transaction commits, + // so a row that leaves the allowlist while the DELETE waits survives. query := ` DELETE FROM purchase_executions WHERE ( @@ -2381,13 +2398,14 @@ func (s *PostgresStore) CleanupOldExecutions(ctx context.Context, retentionDays AND updated_at < NOW() - INTERVAL '1 day' * $1 ) OR ( - status <> ALL($2) + status = ANY($2) + AND status <> ALL($3) AND expires_at IS NOT NULL AND expires_at < NOW() - INTERVAL '1 day' * $1 ) ` - result, err := s.db.Exec(ctx, query, retentionDays, HealthScoredExecutionStatuses) + result, err := s.db.Exec(ctx, query, retentionDays, TerminalExecutionStatuses, HealthScoredExecutionStatuses) if err != nil { return 0, fmt.Errorf("failed to cleanup old executions: %w", err) } diff --git a/internal/config/store_postgres_cleanup_integration_test.go b/internal/config/store_postgres_cleanup_integration_test.go new file mode 100644 index 000000000..04b28c569 --- /dev/null +++ b/internal/config/store_postgres_cleanup_integration_test.go @@ -0,0 +1,99 @@ +//go:build integration + +package config + +import ( + "context" + "regexp" + "slices" + "testing" + + "github.com/stretchr/testify/require" +) + +// cleanupTTLDeletable are the statuses CleanupOldExecutions' expires_at branch +// may delete: settled (nothing acts on them again) and not counted by the plan +// health score. Spelled out literally rather than derived from +// TerminalExecutionStatuses, so a change to either shared list has to be made +// here too, on purpose. +var cleanupTTLDeletable = []string{StatusCompleted, StatusPartiallyCompleted, StatusExpired, StatusRevocationRequested} + +// executionStatusesAllowedBySchema reads the purchase_executions status CHECK, +// so a status added to the schema is covered by the test below without anyone +// having to remember it. +func executionStatusesAllowedBySchema(ctx context.Context, t *testing.T, store *PostgresStore) []string { + t.Helper() + var def string + require.NoError(t, store.db.QueryRow(ctx, `SELECT pg_get_constraintdef(oid) FROM pg_constraint + WHERE conrelid = 'purchase_executions'::regclass AND contype = 'c' + AND pg_get_constraintdef(oid) LIKE '%status%'`).Scan(&def)) + matches := regexp.MustCompile(`'([a-z_]+)'`).FindAllStringSubmatch(def, -1) + out := make([]string, 0, len(matches)) + for _, m := range matches { + out = append(out, m[1]) + } + require.NotEmpty(t, out, "parse the status CHECK: %s", def) + return out +} + +// insertCleanupRow adds a plan-less execution whose expires_at is expiresDays +// from the database clock (nil leaves it NULL). scheduled_date is far in the +// future and updated_at is now, so only the expires_at branch can match it. +func insertCleanupRow(ctx context.Context, t *testing.T, store *PostgresStore, status string, expiresDays *int) string { + t.Helper() + var id string + require.NoError(t, store.db.QueryRow(ctx, ` + INSERT INTO purchase_executions (status, scheduled_date, updated_at, expires_at) + VALUES ($1, NOW() + INTERVAL '400 days', NOW(), NOW() + make_interval(days => $2::int)) + RETURNING execution_id::text`, status, expiresDays).Scan(&id), "insert %s row", status) + return id +} + +// TestPostgresStoreDB_CleanupOldExecutions_ExpiryBranchDeletesOnlySettledRows +// is the regression guard for #120: the expires_at branch used to delete a +// row in any status the health score does not count, so an approved or +// running row stranded by a dead executor, with a lapsed TTL, was deleted +// before GetStaleApprovedExecutions or ListStuckExecutions could resolve it. +// Every status the schema allows gets a lapsed-TTL row; only the settled, +// unscored ones may go. Fail-before: approved, running, pending, notified, +// scheduled and paused rows are deleted. +func TestPostgresStoreDB_CleanupOldExecutions_ExpiryBranchDeletesOnlySettledRows(t *testing.T) { + conn := setupTestContainerDB(t) + cleanupTestData(t, conn) + store := NewPostgresStore(conn) + ctx := context.Background() + + allowed := executionStatusesAllowedBySchema(ctx, t, store) + for _, s := range append([]string{"approved", "running"}, cleanupTTLDeletable...) { + require.Contains(t, allowed, s, "the schema must still allow %s for this test to mean anything", s) + } + + lapsed, inside := -40, -10 + want := map[string]string{} + for _, s := range allowed { + id := insertCleanupRow(ctx, t, store, s, &lapsed) + if !slices.Contains(cleanupTTLDeletable, s) { + want[id] = s + continue + } + // A deletable status still survives with a TTL inside retention or none. + want[insertCleanupRow(ctx, t, store, s, &inside)] = s + want[insertCleanupRow(ctx, t, store, s, nil)] = s + } + + deleted, err := store.CleanupOldExecutions(ctx, 30) + require.NoError(t, err) + require.EqualValues(t, len(cleanupTTLDeletable), deleted, "only one lapsed row per settled, unscored status goes") + + rows, err := store.db.Query(ctx, `SELECT execution_id::text, status FROM purchase_executions`) + require.NoError(t, err) + defer rows.Close() + got := map[string]string{} + for rows.Next() { + var id, status string + require.NoError(t, rows.Scan(&id, &status)) + got[id] = status + } + require.NoError(t, rows.Err()) + require.Equal(t, want, got, "live, health-scored, unexpired and TTL-less rows survive") +} diff --git a/internal/config/store_postgres_db_test.go b/internal/config/store_postgres_db_test.go index d53b7a88a..72c66efa5 100644 --- a/internal/config/store_postgres_db_test.go +++ b/internal/config/store_postgres_db_test.go @@ -1344,8 +1344,17 @@ func TestPostgresStoreDB_CleanupOldExecutions_RetainsCanceledInsideHealthWindow( scheduledDays: 400, updatedDays: 0, expiresDays: intPtr(-40), + shouldSurvive: true, + why: "not settled: ExpireExecutionAtomic moves it to expired first, and only then may a lapsed TTL reap it (#120)", + }, + { + name: "expired-with-long-expired-token", + status: StatusExpired, + scheduledDays: 400, + updatedDays: 0, + expiresDays: intPtr(-40), shouldSurvive: false, - why: "dead approval token; the expires_at branch still reaps non-canceled rows", + why: "settled and not health-scored; the expires_at branch still reaps it", }, { name: "pending-old-scheduled-date-no-expiry", diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 04bbefc12..44910cc91 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -8,6 +8,7 @@ import ( "database/sql" "encoding/json" "errors" + "slices" "strings" "testing" "time" @@ -2634,9 +2635,9 @@ func TestPGXMock_CleanupOldExecutions_Success(t *testing.T) { store := storeWith(mock) ctx := context.Background() - // Two args: retentionDays, then the health-scored status list branch 3 - // excludes (see CleanupOldExecutions). - mock.ExpectExec("DELETE").WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()). + // Three args: retentionDays, then branch 3's settled-status allowlist and + // the health-scored statuses it excludes (see CleanupOldExecutions). + mock.ExpectExec("DELETE").WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("DELETE", 5)) n, err := store.CleanupOldExecutions(ctx, 90) @@ -3006,7 +3007,7 @@ func TestPGXMock_ListAccountRegistrations_SearchEscapesPercent(t *testing.T) { AddRow(minimalRegRow("reg-1", "%foo Corp", "%foo@example.com")...) // The query must contain ESCAPE to prove the clause was updated. mock.ExpectQuery(`ESCAPE`). - WithArgs("%" + escapedArg + "%"). + WithArgs("%"+escapedArg+"%", MaxListLimit). WillReturnRows(rows) filter := AccountRegistrationFilter{Search: rawSearch} @@ -3031,7 +3032,7 @@ func TestPGXMock_ListAccountRegistrations_SearchEscapesUnderscore(t *testing.T) rows := pgxmock.NewRows(registrationCols()). AddRow(minimalRegRow("reg-2", "foo_bar Inc", "foo_bar@example.com")...) mock.ExpectQuery(`ESCAPE`). - WithArgs("%" + escapedArg + "%"). + WithArgs("%"+escapedArg+"%", MaxListLimit). WillReturnRows(rows) filter := AccountRegistrationFilter{Search: rawSearch} @@ -3058,7 +3059,7 @@ func TestPGXMock_ListAccountRegistrations_SearchEscapesBackslash(t *testing.T) { AddRow(minimalRegRow("reg-3", `foo\bar Ltd`, `foo\bar@example.com`)...) // The query must contain ESCAPE to prove the clause was updated. mock.ExpectQuery(`ESCAPE`). - WithArgs("%" + escapedArg + "%"). + WithArgs("%"+escapedArg+"%", MaxListLimit). WillReturnRows(rows) filter := AccountRegistrationFilter{Search: rawSearch} @@ -3281,7 +3282,7 @@ func TestPGXMock_CleanupOldExecutions_IncludesCanonicalStatus(t *testing.T) { // appearing together in the SQL. If either is missing the mock won't // match, causing an "unexpected query" error from ExpectationsWereMet. mock.ExpectExec(`'cancelled'.*'canceled'`). - WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("DELETE", 4)) n, err := store.CleanupOldExecutions(ctx, 90) @@ -3304,7 +3305,7 @@ func TestPGXMock_CleanupOldExecutions_RetainsCanceledOnUpdatedAt(t *testing.T) { anchors := map[string]string{ "canceled branch retains on updated_at": `(?s)status IN \('cancelled', 'canceled'\)\s+AND updated_at`, "completed branch retains on scheduled_date": `(?s)status = 'completed'\s+AND scheduled_date`, - "expires_at branch skips scored statuses": `(?s)status <> ALL\(\$2\)\s+AND expires_at IS NOT NULL`, + "expires_at branch allowlists settled rows": `(?s)status = ANY\(\$2\)\s+AND status <> ALL\(\$3\)\s+AND expires_at IS NOT NULL`, } for name, pattern := range anchors { @@ -3314,7 +3315,7 @@ func TestPGXMock_CleanupOldExecutions_RetainsCanceledOnUpdatedAt(t *testing.T) { ctx := context.Background() mock.ExpectExec(pattern). - WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("DELETE", 1)) n, err := store.CleanupOldExecutions(ctx, 30) @@ -3326,7 +3327,10 @@ func TestPGXMock_CleanupOldExecutions_RetainsCanceledOnUpdatedAt(t *testing.T) { } // TestPGXMock_CleanupOldExecutions_ExcludesEveryHealthScoredStatus pins the -// $2 argument itself, not just the `<> ALL($2)` spelling. The anchors above +// $2 and $3 arguments themselves, not just the `= ANY($2)` and `<> ALL($3)` +// spellings. $2 is the settled-status allowlist (#120): passing a wider list, +// or none, lets a lapsed TTL delete an approved or running row the reaper +// still needs. $3 is the health-score exclusion. The anchors above // would still match if someone passed a narrower list (say, the canceled // spellings only), which is exactly the drift the shared slice exists to // prevent: `failed` is scored at up to -40, so dropping it from the @@ -3339,7 +3343,7 @@ func TestPGXMock_CleanupOldExecutions_ExcludesEveryHealthScoredStatus(t *testing ctx := context.Background() mock.ExpectExec("DELETE"). - WithArgs(30, HealthScoredExecutionStatuses). + WithArgs(30, TerminalExecutionStatuses, HealthScoredExecutionStatuses). WillReturnResult(pgxmock.NewResult("DELETE", 2)) n, err := store.CleanupOldExecutions(ctx, 30) @@ -3350,6 +3354,17 @@ func TestPGXMock_CleanupOldExecutions_ExcludesEveryHealthScoredStatus(t *testing assert.ElementsMatch(t, []string{StatusFailed, StatusCanceled, LegacyStatusCanceled}, HealthScoredExecutionStatuses) + // The statuses a lapsed TTL may delete: settled and not health-scored. + // None of them is one an executor, scheduler or reaper acts on. + var ttlDeletable []string + for _, st := range TerminalExecutionStatuses { + if !slices.Contains(HealthScoredExecutionStatuses, st) { + ttlDeletable = append(ttlDeletable, st) + } + } + assert.ElementsMatch(t, + []string{StatusCompleted, StatusPartiallyCompleted, StatusExpired, StatusRevocationRequested}, + ttlDeletable) } // ─── CountExecutionsByPlanAndStatus ────────────────────────────────────────── diff --git a/internal/config/store_postgres_writer_race_integration_test.go b/internal/config/store_postgres_writer_race_integration_test.go index e88955666..4468ad276 100644 --- a/internal/config/store_postgres_writer_race_integration_test.go +++ b/internal/config/store_postgres_writer_race_integration_test.go @@ -39,14 +39,16 @@ func TestFanOutChildQueuesBehindCommandAndIsAccepted(t *testing.T) { // W21: the prune DELETE rechecks the committed row after the wait. The // fixture ramp is finished (3 of 3): on an unfinished ramp the retention // trigger of migration 000124 already keeps every capacity-holding row, so -// only the SQL predicate would be unexercised. +// only the SQL predicate would be unexercised. The row is expired, the +// status a lapsed approval lands in: since #120 a lapsed TTL deletes only a +// settled row, so a pending one is no longer prunable at all. func TestPruneRechecksRowAfterCommandCancelsIt(t *testing.T) { for _, commandWins := range []bool{true, false} { name := map[bool]string{true: "command first", false: "prune first"}[commandWins] t.Run(name, func(t *testing.T) { ctx := context.Background() f := newRampFixture(ctx, t, 0, 3, 3) - row := f.row(ctx, t, 2, nil, "pending") + row := f.row(ctx, t, 2, nil, StatusExpired) _, err := f.store.db.Exec(ctx, `UPDATE purchase_executions SET expires_at = NOW() - INTERVAL '40 days' WHERE execution_id = $1`, row.ExecutionID) require.NoError(t, err) prune := func() error { _, pErr := f.store.CleanupOldExecutions(ctx, 30); return pErr } From 60845ea6b0e2fa5396121f3a15657e9575ddfbe7 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 27 Oct 2026 23:22:21 +0000 Subject: [PATCH 2/5] fix(db): add range CHECKs for the RI-exchange limits in global_config Migration 000149 adds CHECKs that each RI-exchange USD cap is a finite amount >= 0 and the utilization threshold is in [0, 100], matching GlobalConfig.validateRIExchangeLimits. Each is added NOT VALID and then validated only if no existing row violates it; a violating row is kept, its constraint stays NOT VALID (still checked on every write) and the migration raises a WARNING with the fix. The header gives a read-only query for violating rows. Refs #120 Co-Authored-By: Claude Opus 5.5 --- ...l_config_ri_exchange_limit_checks.down.sql | 5 + ...bal_config_ri_exchange_limit_checks.up.sql | 97 +++++++++ ...al_config_ri_exchange_limit_checks_test.go | 204 ++++++++++++++++++ 3 files changed, 306 insertions(+) create mode 100644 internal/database/postgres/migrations/000149_global_config_ri_exchange_limit_checks.down.sql create mode 100644 internal/database/postgres/migrations/000149_global_config_ri_exchange_limit_checks.up.sql create mode 100644 internal/database/postgres/migrations/000149_global_config_ri_exchange_limit_checks_test.go diff --git a/internal/database/postgres/migrations/000149_global_config_ri_exchange_limit_checks.down.sql b/internal/database/postgres/migrations/000149_global_config_ri_exchange_limit_checks.down.sql new file mode 100644 index 000000000..607a15d5b --- /dev/null +++ b/internal/database/postgres/migrations/000149_global_config_ri_exchange_limit_checks.down.sql @@ -0,0 +1,5 @@ +-- Revert migration 000149: drop the RI-exchange range CHECKs. No data changes. +ALTER TABLE global_config + DROP CONSTRAINT IF EXISTS chk_global_config_ri_exchange_max_per_exchange_usd_range, + DROP CONSTRAINT IF EXISTS chk_global_config_ri_exchange_max_daily_usd_range, + DROP CONSTRAINT IF EXISTS chk_global_config_ri_exchange_utilization_threshold_range; diff --git a/internal/database/postgres/migrations/000149_global_config_ri_exchange_limit_checks.up.sql b/internal/database/postgres/migrations/000149_global_config_ri_exchange_limit_checks.up.sql new file mode 100644 index 000000000..c5c35c035 --- /dev/null +++ b/internal/database/postgres/migrations/000149_global_config_ri_exchange_limit_checks.up.sql @@ -0,0 +1,97 @@ +-- Migration 000149: range CHECKs on the RI-exchange limits in global_config +-- (issue #120, item 2b, database half). +-- +-- 000010 added ri_exchange_max_per_exchange_usd, ri_exchange_max_daily_usd and +-- ri_exchange_utilization_threshold as DOUBLE PRECISION NOT NULL with no range +-- constraint. GlobalConfig.validateRIExchangeLimits (#1297) now rejects bad +-- values on PUT /api/config, but a manual SQL update, an older build still +-- running during a deploy, or a future direct-DB writer could still store a +-- negative money cap, which the exchange guard would then compare spend +-- against. These constraints hold the database to the same bounds: +-- +-- * each USD cap is a finite amount >= 0. 0 stays valid: it is the "not +-- configured" state the exchange guard refuses to run under. The +-- `< 'Infinity'` half matters because a bare `>= 0` admits both +-- 'Infinity' and 'NaN' (PostgreSQL sorts NaN above every other float), and +-- validateRIExchangeLimits rejects both; +-- * the utilization threshold is a percentage in [0, 100]. BETWEEN already +-- rejects NaN and both infinities. +-- +-- ------------------------------------------------------------------ +-- Existing rows that violate a bound +-- ------------------------------------------------------------------ +-- This read-only query lists them (global_config holds one row, id = 1): +-- +-- SELECT id, ri_exchange_max_per_exchange_usd, ri_exchange_max_daily_usd, +-- ri_exchange_utilization_threshold +-- FROM global_config +-- WHERE NOT (ri_exchange_max_per_exchange_usd >= 0 +-- AND ri_exchange_max_per_exchange_usd < 'Infinity'::float8) +-- OR NOT (ri_exchange_max_daily_usd >= 0 +-- AND ri_exchange_max_daily_usd < 'Infinity'::float8) +-- OR NOT (ri_exchange_utilization_threshold BETWEEN 0 AND 100); +-- +-- The migration does NOT fail on such a row and does NOT rewrite it: choosing +-- a money cap is an operator decision, and failing here would leave the +-- database dirty and block every later migration over one config value. Each +-- constraint is added NOT VALID, which enforces it on every INSERT and UPDATE +-- from this point on, and is then VALIDATEd only if no existing row violates +-- it. A constraint with a violating row stays NOT VALID and the migration +-- raises a WARNING naming it. To finish, set the value through +-- PUT /api/config (or an UPDATE) to an amount in range, then run: +-- +-- ALTER TABLE global_config VALIDATE CONSTRAINT ; +-- +-- or re-run this file, which is idempotent. Constraints still unvalidated: +-- +-- SELECT conname FROM pg_constraint +-- WHERE conrelid = 'global_config'::regclass AND NOT convalidated; +-- +-- Until then the only effect of the bad row is that a write which keeps the +-- bad value is refused, and validateRIExchangeLimits already refuses those. +-- +-- ------------------------------------------------------------------ +-- Concurrency +-- ------------------------------------------------------------------ +-- The file runs as one transaction (see migration_transactional_test.go). The +-- first ADD CONSTRAINT takes ACCESS EXCLUSIVE on global_config and holds it to +-- commit, so no write lands between adding a constraint, checking for +-- violating rows and validating it. A config write already in flight finishes +-- first and is then seen by the check; one that arrives later waits and is +-- checked against the new constraints. global_config is a single row, so the +-- lock is brief. +-- +-- The pg_constraint guards follow 000048, so a re-run neither fails on an +-- existing constraint nor re-adds it. + +DO $$ +DECLARE + c RECORD; + violated BOOLEAN; +BEGIN + FOR c IN + SELECT * FROM (VALUES + ('chk_global_config_ri_exchange_max_per_exchange_usd_range', + 'ri_exchange_max_per_exchange_usd >= 0 AND ri_exchange_max_per_exchange_usd < ''Infinity''::float8'), + ('chk_global_config_ri_exchange_max_daily_usd_range', + 'ri_exchange_max_daily_usd >= 0 AND ri_exchange_max_daily_usd < ''Infinity''::float8'), + ('chk_global_config_ri_exchange_utilization_threshold_range', + 'ri_exchange_utilization_threshold BETWEEN 0 AND 100') + ) AS v(name, expr) + LOOP + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conrelid = 'global_config'::regclass AND conname = c.name + ) THEN + EXECUTE format('ALTER TABLE global_config ADD CONSTRAINT %I CHECK (%s) NOT VALID', c.name, c.expr); + END IF; + + EXECUTE format('SELECT EXISTS (SELECT 1 FROM global_config WHERE NOT (%s))', c.expr) INTO violated; + IF violated THEN + RAISE WARNING 'global_config violates % (%); left NOT VALID, so new writes are checked but the existing row is kept', c.name, c.expr + USING HINT = 'Set the value in range (PUT /api/config or UPDATE global_config), then run: ALTER TABLE global_config VALIDATE CONSTRAINT ' || c.name; + ELSE + EXECUTE format('ALTER TABLE global_config VALIDATE CONSTRAINT %I', c.name); + END IF; + END LOOP; +END $$; diff --git a/internal/database/postgres/migrations/000149_global_config_ri_exchange_limit_checks_test.go b/internal/database/postgres/migrations/000149_global_config_ri_exchange_limit_checks_test.go new file mode 100644 index 000000000..34cf364a5 --- /dev/null +++ b/internal/database/postgres/migrations/000149_global_config_ri_exchange_limit_checks_test.go @@ -0,0 +1,204 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgconn" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" +) + +const ( + m149Version = 149 + m149UpFile = "000149_global_config_ri_exchange_limit_checks.up.sql" + m149DownFile = "000149_global_config_ri_exchange_limit_checks.down.sql" + m149PerExCap = "chk_global_config_ri_exchange_max_per_exchange_usd_range" + m149DailyCap = "chk_global_config_ri_exchange_max_daily_usd_range" + m149Threshold = "chk_global_config_ri_exchange_utilization_threshold_range" +) + +// m149ValidatedState maps each 000149 constraint present on global_config to +// whether it is validated. +func m149ValidatedState(ctx context.Context, t *testing.T, pool *pgxpool.Pool) map[string]bool { + t.Helper() + rows, err := pool.Query(ctx, `SELECT conname, convalidated FROM pg_constraint + WHERE conrelid = 'global_config'::regclass AND conname = ANY($1)`, + []string{m149PerExCap, m149DailyCap, m149Threshold}) + require.NoError(t, err) + defer rows.Close() + out := map[string]bool{} + for rows.Next() { + var name string + var validated bool + require.NoError(t, rows.Scan(&name, &validated)) + out[name] = validated + } + require.NoError(t, rows.Err()) + return out +} + +// m149Set writes one RI-exchange column of the single global_config row. +// value is text so 'NaN' and 'Infinity' can be written. +func m149Set(ctx context.Context, pool *pgxpool.Pool, column, value string) error { + _, err := pool.Exec(ctx, `UPDATE global_config SET `+column+` = $1::float8 WHERE id = 1`, value) + return err +} + +func m149RequireCheckViolation(t *testing.T, err error, constraint, msg string) { + t.Helper() + var pgErr *pgconn.PgError + require.True(t, errors.As(err, &pgErr), "%s: want a PostgreSQL error, got %v", msg, err) + assert.Equal(t, "23514", pgErr.Code, "%s: want check_violation", msg) + assert.Equal(t, constraint, pgErr.ConstraintName, msg) +} + +func m149MigrateToPrevious(ctx context.Context, t *testing.T) (*pgxpool.Pool, string) { + t.Helper() + container := testhelpers.RequireEmptyDatabase(ctx, t) + pool, path := container.DB.Pool(), getMigrationsPath() + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, previousMigrationVersion(t, m149Version))) + _, err := pool.Exec(ctx, `INSERT INTO global_config (id) VALUES (1) ON CONFLICT (id) DO NOTHING`) + require.NoError(t, err) + return pool, path +} + +// TestMigration149_EnforcesRIExchangeLimitRanges: on a clean row every +// constraint is added and validated, out-of-range writes (negative, NaN, +// infinite caps; a threshold outside [0, 100]) are refused and the bounds +// themselves are accepted. The file is idempotent and the down drops all three. +// +// Fail-before: at the previous version a negative cap is stored. +func TestMigration149_EnforcesRIExchangeLimitRanges(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + pool, path := m149MigrateToPrevious(ctx, t) + + require.NoError(t, m149Set(ctx, pool, "ri_exchange_max_daily_usd", "-1"), "precondition: no range check before 000149") + require.NoError(t, m149Set(ctx, pool, "ri_exchange_max_daily_usd", "0")) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, m149Version)) + all := map[string]bool{m149PerExCap: true, m149DailyCap: true, m149Threshold: true} + require.Equal(t, all, m149ValidatedState(ctx, t, pool), "a clean row lets every constraint validate") + + refused := []struct{ column, value, constraint string }{ + {"ri_exchange_max_per_exchange_usd", "-1", m149PerExCap}, + {"ri_exchange_max_per_exchange_usd", "-0.000001", m149PerExCap}, + {"ri_exchange_max_per_exchange_usd", "Infinity", m149PerExCap}, + {"ri_exchange_max_per_exchange_usd", "NaN", m149PerExCap}, + {"ri_exchange_max_daily_usd", "-1", m149DailyCap}, + {"ri_exchange_max_daily_usd", "Infinity", m149DailyCap}, + {"ri_exchange_max_daily_usd", "NaN", m149DailyCap}, + {"ri_exchange_utilization_threshold", "-0.1", m149Threshold}, + {"ri_exchange_utilization_threshold", "100.1", m149Threshold}, + {"ri_exchange_utilization_threshold", "NaN", m149Threshold}, + {"ri_exchange_utilization_threshold", "-Infinity", m149Threshold}, + } + for _, r := range refused { + m149RequireCheckViolation(t, m149Set(ctx, pool, r.column, r.value), r.constraint, r.column+" = "+r.value) + } + accepted := []struct{ column, value string }{ + {"ri_exchange_max_per_exchange_usd", "0"}, + {"ri_exchange_max_per_exchange_usd", "1e12"}, + {"ri_exchange_max_daily_usd", "0"}, + {"ri_exchange_max_daily_usd", "25000.50"}, + {"ri_exchange_utilization_threshold", "0"}, + {"ri_exchange_utilization_threshold", "100"}, + } + for _, a := range accepted { + require.NoError(t, m149Set(ctx, pool, a.column, a.value), "%s = %s is in range", a.column, a.value) + } + + up, err := os.ReadFile(filepath.Join(path, m149UpFile)) + require.NoError(t, err) + _, err = pool.Exec(ctx, string(up)) + require.NoError(t, err, "the up file is idempotent") + require.Equal(t, all, m149ValidatedState(ctx, t, pool)) + + down, err := os.ReadFile(filepath.Join(path, m149DownFile)) + require.NoError(t, err) + _, err = pool.Exec(ctx, string(down)) + require.NoError(t, err) + require.Empty(t, m149ValidatedState(ctx, t, pool), "the down drops all three") + require.NoError(t, m149Set(ctx, pool, "ri_exchange_max_daily_usd", "-1"), "no range check after the down") +} + +// TestMigration149_KeepsAndReportsAnExistingBadRow: a row already out of range +// does not fail the migration and is not rewritten. Its constraints stay +// NOT VALID, which still refuses any write that keeps a bad value, and the +// clean column's constraint validates. Once the value is fixed, re-running +// the file validates the rest. +func TestMigration149_KeepsAndReportsAnExistingBadRow(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + pool, path := m149MigrateToPrevious(ctx, t) + + require.NoError(t, m149Set(ctx, pool, "ri_exchange_max_daily_usd", "-5")) + require.NoError(t, m149Set(ctx, pool, "ri_exchange_utilization_threshold", "150")) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, m149Version), "a bad row must not fail the migration") + require.Equal(t, map[string]bool{m149PerExCap: true, m149DailyCap: false, m149Threshold: false}, + m149ValidatedState(ctx, t, pool)) + + var daily, threshold float64 + require.NoError(t, pool.QueryRow(ctx, `SELECT ri_exchange_max_daily_usd, ri_exchange_utilization_threshold + FROM global_config WHERE id = 1`).Scan(&daily, &threshold)) + assert.Equal(t, -5.0, daily, "the operator's value is kept, not rewritten") + assert.Equal(t, 150.0, threshold) + + m149RequireCheckViolation(t, m149Set(ctx, pool, "ri_exchange_max_per_exchange_usd", "10"), m149DailyCap, + "NOT VALID still checks every updated row, so a write keeping the bad cap is refused") + m149RequireCheckViolation(t, m149Set(ctx, pool, "ri_exchange_max_daily_usd", "-6"), m149DailyCap, "a new bad value") + + _, err := pool.Exec(ctx, `UPDATE global_config SET ri_exchange_max_daily_usd = 0, ri_exchange_utilization_threshold = 95 WHERE id = 1`) + require.NoError(t, err, "fixing both values in one write is accepted") + up, err := os.ReadFile(filepath.Join(path, m149UpFile)) + require.NoError(t, err) + _, err = pool.Exec(ctx, string(up)) + require.NoError(t, err) + require.Equal(t, map[string]bool{m149PerExCap: true, m149DailyCap: true, m149Threshold: true}, + m149ValidatedState(ctx, t, pool), "re-running the file after the fix validates the rest") +} + +// TestMigration149_ConcurrentConfigWriter: a config write in flight when the +// migration starts holds the row; the migration waits for it, then sees the +// committed value. A bad value committed that way is kept and its constraint +// left NOT VALID rather than validated over a violating row or failing the +// migration, and the next bad write is refused. +func TestMigration149_ConcurrentConfigWriter(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + pool, path := m149MigrateToPrevious(ctx, t) + + writer, err := pool.Begin(ctx) + require.NoError(t, err) + t.Cleanup(func() { _ = writer.Rollback(context.Background()) }) + _, err = writer.Exec(ctx, `UPDATE global_config SET ri_exchange_max_per_exchange_usd = -3 WHERE id = 1`) + require.NoError(t, err) + + done := make(chan error, 1) + go func() { done <- migrations.MigrateToVersion(ctx, pool, path, m149Version) }() + // pg_stat_activity keeps only the first 1024 bytes of a query, so match + // the file's first line rather than a constraint name further down. + testhelpers.WaitForLockWait(ctx, t, pool, "-- Migration 000149:%") + require.NoError(t, writer.Commit(ctx)) + require.NoError(t, testhelpers.Await(t, done), "the migration waits for the writer, then completes") + + require.Equal(t, map[string]bool{m149PerExCap: false, m149DailyCap: true, m149Threshold: true}, + m149ValidatedState(ctx, t, pool), "the value committed while the migration waited is seen") + m149RequireCheckViolation(t, m149Set(ctx, pool, "ri_exchange_max_per_exchange_usd", "-4"), m149PerExCap, + "a bad write after the migration is refused") +} From 6f38ebcf49779a77849d23d0d034227b70331df9 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 27 Oct 2026 23:22:21 +0000 Subject: [PATCH 3/5] fix(config): cap registration, ladder config and suppression list reads ListAccountRegistrations and GetLadderConfigs now clamp to MaxListLimit like the package's other list reads, with id as a tie-break so the cut is deterministic. ListActiveSuppressions returns an error above the cap instead of truncating: a partial list would under-suppress, and the money path (ListRecommendationsStrict) would offer to re-buy the dropped rows. Refs #120 Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres_ladder.go | 10 +- .../config/store_postgres_list_caps_test.go | 110 ++++++++++++++++++ .../config/store_postgres_registrations.go | 7 +- .../config/store_postgres_suppressions.go | 15 ++- .../store_postgres_suppressions_test.go | 1 + 5 files changed, 137 insertions(+), 6 deletions(-) create mode 100644 internal/config/store_postgres_list_caps_test.go diff --git a/internal/config/store_postgres_ladder.go b/internal/config/store_postgres_ladder.go index 7c171158d..90ffed86d 100644 --- a/internal/config/store_postgres_ladder.go +++ b/internal/config/store_postgres_ladder.go @@ -17,7 +17,10 @@ import ( // LADDER CONFIG STORE METHODS // ========================================== -// GetLadderConfigs returns all ladder_configs rows, newest first. +// GetLadderConfigs returns the ladder_configs rows, newest first, capped at +// MaxListLimit like the package's other list reads (#120). There is one row +// per (cloud_account_id, provider), so the cap is only reached with more +// than a thousand account/provider pairs. // Returns an empty slice (not nil) when no rows exist. func (s *PostgresStore) GetLadderConfigs(ctx context.Context) ([]LadderConfigDB, error) { query := ` @@ -27,9 +30,10 @@ func (s *PostgresStore) GetLadderConfigs(ctx context.Context) ([]LadderConfigDB, max_hourly_commit_per_run, max_actions_per_run, ramp_schedule, created_at, updated_at FROM ladder_configs - ORDER BY created_at DESC + ORDER BY created_at DESC, id + LIMIT $1 ` - rows, err := s.db.Query(ctx, query) + rows, err := s.db.Query(ctx, query, MaxListLimit) if err != nil { return nil, fmt.Errorf("failed to query ladder_configs: %w", err) } diff --git a/internal/config/store_postgres_list_caps_test.go b/internal/config/store_postgres_list_caps_test.go new file mode 100644 index 000000000..49a183212 --- /dev/null +++ b/internal/config/store_postgres_list_caps_test.go @@ -0,0 +1,110 @@ +package config + +// Unbounded list reads clamped to MaxListLimit (#120). Each test pins the +// bound LIMIT argument, so dropping the LIMIT (no argument bound) or raising +// it fails the mock's argument match. + +import ( + "context" + "testing" + "time" + + "github.com/pashagolub/pgxmock/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPGXMock_ListAccountRegistrations_ClampsToMaxListLimit(t *testing.T) { + status, provider := "pending", "aws" + cases := []struct { + name string + filter AccountRegistrationFilter + pattern string + args []any + }{ + {"no filter", AccountRegistrationFilter{}, `ORDER BY created_at DESC, id LIMIT \$1$`, []any{MaxListLimit}}, + {"status and provider", AccountRegistrationFilter{Status: &status, Provider: &provider}, + `status = \$1 AND provider = \$2 ORDER BY created_at DESC, id LIMIT \$3$`, []any{status, provider, MaxListLimit}}, + {"every filter", AccountRegistrationFilter{Status: &status, Provider: &provider, Search: "acme"}, + `ESCAPE '\\'\) ORDER BY created_at DESC, id LIMIT \$4$`, []any{status, provider, "%acme%", MaxListLimit}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + mock.ExpectQuery(tc.pattern).WithArgs(tc.args...). + WillReturnRows(pgxmock.NewRows(registrationCols())) + + regs, err := store.ListAccountRegistrations(context.Background(), tc.filter) + require.NoError(t, err) + assert.Empty(t, regs) + assert.NoError(t, mock.ExpectationsWereMet()) + }) + } +} + +func TestPGXMock_GetLadderConfigs_ClampsToMaxListLimit(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + cols := []string{ + "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", + } + mock.ExpectQuery(`(?s)FROM ladder_configs\s+ORDER BY created_at DESC, id\s+LIMIT \$1`). + WithArgs(MaxListLimit). + WillReturnRows(pgxmock.NewRows(cols)) + + configs, err := store.GetLadderConfigs(context.Background()) + require.NoError(t, err) + assert.Equal(t, []LadderConfigDB{}, configs, "no rows still reads as an empty slice, not nil") + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// suppressionRows returns n active suppression rows in the column order +// ListActiveSuppressions scans. +func suppressionRows(n int) *pgxmock.Rows { + now := time.Now().UTC() + rows := pgxmock.NewRows([]string{ + "id", "execution_id", "account_id", "provider", "service", "region", + "resource_type", "engine", "suppressed_count", "expires_at", "created_at", + "availability_zone", "platform", "tenancy", + }) + for i := 0; i < n; i++ { + rows.AddRow("sup", "exec", "acct", "aws", "ec2", "us-east-1", + "t4g.nano", "", 1, now.Add(time.Hour), now, nil, nil, nil) + } + return rows +} + +// A truncated suppression list would under-suppress, and the money path +// would offer to re-buy what the dropped rows cover, so above the cap the +// read fails instead. Exactly the cap is still a complete list. +func TestPGXMock_ListActiveSuppressions_ErrorsAboveMaxListLimit(t *testing.T) { + for _, tc := range []struct { + rows int + wantErr bool + }{ + {MaxListLimit, false}, + {MaxListLimit + 1, true}, + } { + mock := newMock(t) + store := storeWith(mock) + mock.ExpectQuery(`(?s)FROM purchase_suppressions\s+WHERE expires_at > NOW\(\)\s+LIMIT \$1`). + WithArgs(MaxListLimit + 1). + WillReturnRows(suppressionRows(tc.rows)) + + sups, err := store.ListActiveSuppressions(context.Background()) + if tc.wantErr { + require.Error(t, err, "%d rows", tc.rows) + assert.Contains(t, err.Error(), "more than 1000 active suppressions") + assert.Nil(t, sups, "no partial list alongside the error") + } else { + require.NoError(t, err, "%d rows", tc.rows) + assert.Len(t, sups, tc.rows) + } + assert.NoError(t, mock.ExpectationsWereMet()) + } +} diff --git a/internal/config/store_postgres_registrations.go b/internal/config/store_postgres_registrations.go index 0cfc22505..563234ab2 100644 --- a/internal/config/store_postgres_registrations.go +++ b/internal/config/store_postgres_registrations.go @@ -116,7 +116,8 @@ func (s *PostgresStore) GetAccountRegistrationByToken(ctx context.Context, token return s.scanRegistration(ctx, query, token) } -// ListAccountRegistrations returns registrations matching the filter. +// ListAccountRegistrations returns registrations matching the filter, newest +// first, capped at MaxListLimit like the package's other list reads (#120). func (s *PostgresStore) ListAccountRegistrations(ctx context.Context, filter AccountRegistrationFilter) ([]AccountRegistration, error) { var conditions []string var args []interface{} @@ -140,13 +141,15 @@ func (s *PostgresStore) ListAccountRegistrations(ctx context.Context, filter Acc idx, idx, idx, )) args = append(args, "%"+escaped+"%") + idx++ } query := `SELECT ` + registrationColumns() + ` FROM account_registrations` if len(conditions) > 0 { query += " WHERE " + strings.Join(conditions, " AND ") } - query += " ORDER BY created_at DESC" + query += fmt.Sprintf(" ORDER BY created_at DESC, id LIMIT $%d", idx) + args = append(args, MaxListLimit) rows, err := s.db.Query(ctx, query, args...) if err != nil { diff --git a/internal/config/store_postgres_suppressions.go b/internal/config/store_postgres_suppressions.go index 7356c978f..3cadcac71 100644 --- a/internal/config/store_postgres_suppressions.go +++ b/internal/config/store_postgres_suppressions.go @@ -156,6 +156,15 @@ func (s *PostgresStore) DeleteSuppressionsByExecutionTx(ctx context.Context, tx // matches each row's variant (availability zone, platform, tenancy; // issue #816) to subtract cumulative suppressed counts from its rec-list // output. A row with NULL variant columns comes back with a nil Variant. +// +// More than MaxListLimit active rows is an error, not a truncated list +// (#120). Every row subtracts from a rec's count, so a truncated list would +// silently under-suppress an arbitrary subset of recently purchased +// capacity, and the money path (scheduler.ListRecommendationsStrict) would +// then offer to re-buy it. An error is what every caller already handles as +// "could not read suppressions": the money path refuses, the read-only pages +// log it and over-show. The query fetches one row past the cap to tell the +// two cases apart without a second COUNT round trip. func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseSuppression, error) { const q = ` SELECT id, execution_id, account_id, provider, service, region, @@ -163,8 +172,9 @@ func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseS availability_zone, platform, tenancy FROM purchase_suppressions WHERE expires_at > NOW() + LIMIT $1 ` - rows, err := s.db.Query(ctx, q) + rows, err := s.db.Query(ctx, q, MaxListLimit+1) if err != nil { return nil, fmt.Errorf("failed to query active suppressions: %w", err) } @@ -188,6 +198,9 @@ func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseS if err := rows.Err(); err != nil { return nil, fmt.Errorf("failed to iterate suppression rows: %w", err) } + if len(out) > MaxListLimit { + return nil, fmt.Errorf("more than %d active suppressions; refusing to return a partial list", MaxListLimit) + } return out, nil } diff --git a/internal/config/store_postgres_suppressions_test.go b/internal/config/store_postgres_suppressions_test.go index 6779f66be..c59fe471c 100644 --- a/internal/config/store_postgres_suppressions_test.go +++ b/internal/config/store_postgres_suppressions_test.go @@ -129,6 +129,7 @@ func TestPGXMock_ListActiveSuppressions(t *testing.T) { AddRow("sup-2", "exec-2", "acct-1", "aws", "ec2", "us-east-1", "t4g.nano", "", 2, future, now, &az, &platform, &tenancy) mock.ExpectQuery(`SELECT .* FROM purchase_suppressions WHERE expires_at > NOW\(\)`). + WithArgs(MaxListLimit + 1). WillReturnRows(rows) sups, err := store.ListActiveSuppressions(ctx) From 50fec5ef5478ac2537f4e6b486095c072784b199 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Wed, 28 Oct 2026 00:19:39 +0000 Subject: [PATCH 4/5] fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) govulncheck in the Docker image build now fails on these two advisories, both fixed in x/net v0.62.0. Co-Authored-By: Claude Opus 5.5 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 5fb3a2f61..abcf4fb6c 100644 --- a/go.mod +++ b/go.mod @@ -59,7 +59,7 @@ require ( go.opentelemetry.io/otel/metric v1.45.0 // indirect go.opentelemetry.io/otel/trace v1.45.0 // indirect golang.org/x/crypto v0.58.0 - golang.org/x/net v0.61.0 // indirect + golang.org/x/net v0.62.0 // indirect golang.org/x/oauth2 v0.37.0 golang.org/x/sync v0.24.0 golang.org/x/sys v0.49.0 // indirect diff --git a/go.sum b/go.sum index 06deac0b9..83df52e66 100644 --- a/go.sum +++ b/go.sum @@ -367,8 +367,8 @@ golang.org/x/crypto v0.58.0 h1:COkYLr4k7nDI4r6QtMML2AfzeIswWHAJthLS6K/M/54= golang.org/x/crypto v0.58.0/go.mod h1:Xh+kl5A+M0gMiWDytH2Hhr5Z7WbNCpv/eB/EFgfSk+w= golang.org/x/mod v0.42.0 h1:ICeL3t2L30ljSPHHxDnR4fqVcBt/zR2zJAb9juavmQg= golang.org/x/mod v0.42.0/go.mod h1:u5fzIrRInkL8MaO1cbRkUHT64v303YIT5KEqpNr7jRs= -golang.org/x/net v0.61.0 h1:RR+6j/BTBCrggOLugxa+66R8FvoN6MHlzDlZIbN9YFw= -golang.org/x/net v0.61.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= +golang.org/x/net v0.62.0 h1:18ELalfM5+XNRwzM2nYqXsZQ9ByS1uhj2O/U7+5jqRY= +golang.org/x/net v0.62.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA= From e12f22f60794cff2834e87759c5b35f7f2f179d7 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Wed, 28 Oct 2026 02:31:27 +0000 Subject: [PATCH 5/5] fix(config): keep ListActiveSuppressions unbounded One purchase can write up to 1000 suppression rows that live 7-90 days, so normal use can exceed MaxListLimit. Erroring above the cap disabled the suppression index for every caller (automated plan steps refuse; the read-only pages over-show just-bought capacity). Restore main's unbounded read and pin it with a test. Raised in review. Refs #120 Co-Authored-By: Claude Opus 5.5 --- .../config/store_postgres_list_caps_test.go | 38 ++++++------------- .../config/store_postgres_suppressions.go | 18 +++------ .../store_postgres_suppressions_test.go | 1 - 3 files changed, 16 insertions(+), 41 deletions(-) diff --git a/internal/config/store_postgres_list_caps_test.go b/internal/config/store_postgres_list_caps_test.go index 49a183212..2b98152ca 100644 --- a/internal/config/store_postgres_list_caps_test.go +++ b/internal/config/store_postgres_list_caps_test.go @@ -79,32 +79,16 @@ func suppressionRows(n int) *pgxmock.Rows { return rows } -// A truncated suppression list would under-suppress, and the money path -// would offer to re-buy what the dropped rows cover, so above the cap the -// read fails instead. Exactly the cap is still a complete list. -func TestPGXMock_ListActiveSuppressions_ErrorsAboveMaxListLimit(t *testing.T) { - for _, tc := range []struct { - rows int - wantErr bool - }{ - {MaxListLimit, false}, - {MaxListLimit + 1, true}, - } { - mock := newMock(t) - store := storeWith(mock) - mock.ExpectQuery(`(?s)FROM purchase_suppressions\s+WHERE expires_at > NOW\(\)\s+LIMIT \$1`). - WithArgs(MaxListLimit + 1). - WillReturnRows(suppressionRows(tc.rows)) +// ListActiveSuppressions stays unbounded: a large tenant can hold more than +// MaxListLimit active rows, and every row must reach the suppression index. +func TestPGXMock_ListActiveSuppressions_ReturnsEveryRowAboveMaxListLimit(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + mock.ExpectQuery(`(?s)FROM purchase_suppressions\s+WHERE expires_at > NOW\(\)\s*$`). + WillReturnRows(suppressionRows(MaxListLimit + 5)) - sups, err := store.ListActiveSuppressions(context.Background()) - if tc.wantErr { - require.Error(t, err, "%d rows", tc.rows) - assert.Contains(t, err.Error(), "more than 1000 active suppressions") - assert.Nil(t, sups, "no partial list alongside the error") - } else { - require.NoError(t, err, "%d rows", tc.rows) - assert.Len(t, sups, tc.rows) - } - assert.NoError(t, mock.ExpectationsWereMet()) - } + sups, err := store.ListActiveSuppressions(context.Background()) + require.NoError(t, err) + assert.Len(t, sups, MaxListLimit+5) + assert.NoError(t, mock.ExpectationsWereMet()) } diff --git a/internal/config/store_postgres_suppressions.go b/internal/config/store_postgres_suppressions.go index 3cadcac71..5395929dd 100644 --- a/internal/config/store_postgres_suppressions.go +++ b/internal/config/store_postgres_suppressions.go @@ -157,14 +157,10 @@ func (s *PostgresStore) DeleteSuppressionsByExecutionTx(ctx context.Context, tx // issue #816) to subtract cumulative suppressed counts from its rec-list // output. A row with NULL variant columns comes back with a nil Variant. // -// More than MaxListLimit active rows is an error, not a truncated list -// (#120). Every row subtracts from a rec's count, so a truncated list would -// silently under-suppress an arbitrary subset of recently purchased -// capacity, and the money path (scheduler.ListRecommendationsStrict) would -// then offer to re-buy it. An error is what every caller already handles as -// "could not read suppressions": the money path refuses, the read-only pages -// log it and over-show. The query fetches one row past the cap to tell the -// two cases apart without a second COUNT round trip. +// Deliberately unbounded (#120 review): every active row subtracts from a +// rec's count, and one purchase can write up to 1000 rows that live 7-90 +// days, so normal use can exceed MaxListLimit. A capped read would either +// under-suppress (re-offering capacity just bought) or fail every caller. func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseSuppression, error) { const q = ` SELECT id, execution_id, account_id, provider, service, region, @@ -172,9 +168,8 @@ func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseS availability_zone, platform, tenancy FROM purchase_suppressions WHERE expires_at > NOW() - LIMIT $1 ` - rows, err := s.db.Query(ctx, q, MaxListLimit+1) + rows, err := s.db.Query(ctx, q) if err != nil { return nil, fmt.Errorf("failed to query active suppressions: %w", err) } @@ -198,9 +193,6 @@ func (s *PostgresStore) ListActiveSuppressions(ctx context.Context) ([]PurchaseS if err := rows.Err(); err != nil { return nil, fmt.Errorf("failed to iterate suppression rows: %w", err) } - if len(out) > MaxListLimit { - return nil, fmt.Errorf("more than %d active suppressions; refusing to return a partial list", MaxListLimit) - } return out, nil } diff --git a/internal/config/store_postgres_suppressions_test.go b/internal/config/store_postgres_suppressions_test.go index c59fe471c..6779f66be 100644 --- a/internal/config/store_postgres_suppressions_test.go +++ b/internal/config/store_postgres_suppressions_test.go @@ -129,7 +129,6 @@ func TestPGXMock_ListActiveSuppressions(t *testing.T) { AddRow("sup-2", "exec-2", "acct-1", "aws", "ec2", "us-east-1", "t4g.nano", "", 2, future, now, &az, &platform, &tenancy) mock.ExpectQuery(`SELECT .* FROM purchase_suppressions WHERE expires_at > NOW\(\)`). - WithArgs(MaxListLimit + 1). WillReturnRows(rows) sups, err := store.ListActiveSuppressions(ctx)