[IAmJSD]

First slice of #120's medium/low internal/config and DB findings. I re-checked every finding against current main (a14dc8f2). This PR fixes four of them, each in its own commit with a fail-before/pass-after test. The rest are deferred below with reasons, mostly because an open PR or a queued branch already edits the same lines.

Refs #120

Per-finding status

#	Finding	Status
1	MEDIUM: rec-detail hidden_by omits min_count	Fixed (d7c2b4b2)
2	MEDIUM: RI-exchange money caps never validated; a 0 threshold rewritten to 95	Validation half fixed (d716b6b6). The coercion half and the DB CHECKs are deferred (see below)
3	MEDIUM: changing external_id orphans purchase history	Deferred
4	MEDIUM: nullable total_usage summed as zero in rollups	Deferred: in flight on queued branch fix/issue-151 (migration 000147 savings_rollups_propagate_unknown)
5	MEDIUM: three copies of the dual-column account predicate	Deferred
6	MEDIUM: ri_utilization_cache PK has no account dimension	Deferred
7	MEDIUM: unbounded list reads	Deferred
8	LOW: GetAccountCredential returns ("", nil) for a missing row	Fixed (2ad49ecb)
9	LOW: CleanupOldExecutions expiry branch deletes rows in any status	Deferred: same function and comment block as queued branch fix/issue-47
10	LOW: TryAdvisoryLock overwrites a pinned connection	Not reproducible; no change
11	LOW: malformed DB tuning env vars fall back to defaults silently	Fixed (1eb95c5f)

None of the eleven was already fixed on main. git log --grep "#120" only matches ab3bac1e, which is about an older #120 (inline Plan button). #1173 fixed finding 9's bug, but only in cmd/cleanup-lambda (deleteExpired, for issue 998). The store's CleanupOldExecutions, which internal/server/handler.go:310 calls, still has the unrestricted branch.

What changed

1. hidden_by and min_count (internal/scheduler/scheduler_overrides.go, scheduler.go). filterRecsByResolvedConfigs dropped a rec on five dimensions, but overrideHiddenReasons checked only four. So a rec under a min_count floor disappeared from the list and then opened by ID with hidden_by = nil. Following the issue's fix direction, both functions now walk one overrideDimensions table, and min_count keeps the Savings Plans carve-out from #1146. A future dimension cannot reach only one of the two. Doc comments on GetRecommendationByID and api.RecommendationDetailResponse.HiddenBy now list min_count.

This PR changes one row of an existing test: TestGlobalFiltersPersistedAmbientAndRegistered/minimum (b4587190). It expected an empty hidden_by for the count-1 recs that the same subtest shows the list hiding. I think that row recorded the bug rather than an intended rule:

• Since 7be8bc5, hidden_by explains global service-config filters, not only per-account overrides. Every other row in that table is a global policy, and each expects its dimension in hidden_by. handler_recommendations_global_filters_integration_test.go likewise expects ["enabled=false"] from a global config.
• min_count can only come from the service config. AccountServiceOverride has no MinCount, and the resolver copies baseline.MinCount. So "report min_count only when an override set it" would never report it.
• The empty expectation was written before the explainer knew about min_count. 585c491 added min_count to the filter without touching the explainer and gave no reason for leaving it out.

2. RI-exchange limits on PUT /api/config (internal/config/validation.go). GlobalConfig.Validate, the only validation PUT /api/config runs on its whole-struct unmarshal, now rejects:

• a negative, NaN or infinite ri_exchange_max_per_exchange_usd or ri_exchange_max_daily_usd
• a ri_exchange_utilization_threshold outside [0, 100]

These are the bounds PUT /api/ri-exchange/config's body already enforces, so both routes now accept the same values. 0 stays valid for all three: a 0 cap is the "not configured" state the exchange guard refuses to run under.

8. GetAccountCredential (internal/config/store_postgres.go, interfaces.go). A missing row now returns ErrNotFound instead of ("", nil), which is what a stored empty blob returns. The method still has no production callers, so no behavior changes today. The existing pgxmock test asserted the old ("", nil) and now asserts ErrNotFound.

11. DB env parsing (internal/database/config.go). getEnvInt, getEnvBool and getEnvDuration are now methods on a small envParser that records every set but unparseable value. LoadFromEnv returns one errors.Join error naming all of them, so DB_MAX_CONNECTIONS="100 ", DB_MAX_CONN_LIFETIME=1h30 or DB_AUTO_MIGRATE=yes fail at boot instead of quietly becoming 25, 1h and false. Unset variables still use the defaults. I checked every value the IaC sets (Terraform for AWS Lambda and Fargate, GCP Cloud Run and GKE, Azure AKS and Container Apps, the cleanup functions), plus docker-compose, the Dockerfile and CI. All of them parse: "8s", "5432", tostring(bool), tostring(number).

Deferred, and why

• 2 (second half): 0 to 95 coercion, plus DB CHECKs. To stop the coercion, "unset" has to be told apart from 0. That means a pointer field in GlobalConfig (internal/config/types.go, which many queued branches edit) and an owner decision on what a threshold of 0 should mean. The CHECK constraints need a migration, and for existing rows a repair step or NOT VALID. Since validation now covers both write routes, I left both parts for a follow-up slice.
• 3: external_id change orphans history. This needs an owner decision: reject the change when history exists, or rewrite purchase_history.account_id and savings_snapshots.account_id in the same transaction. It also overlaps chore(db): backfill purchase_history.cloud_account_id from (provider, external_id) #1201 (fix/issue-22, backfill cloud_account_id) and fix(history): set cloud_account_id on every purchase_history insert #1244 (fix/issue-1235, set cloud_account_id on every insert), which shrink the affected set.
• 4: nullable total_usage rollups. Already being fixed on the queued branch fix/issue-151 (migration 000147).
• 5: three copies of the account predicate. Hoisting the predicate rewrites appendAccountPredicate and sortedProviderKeys in store_postgres.go. Queued branches fix/issue-24 (edits those functions), fix/issue-674 and fix/issue-33 (new callers) touch the same code. It should land after them.
• 6: ri_utilization_cache key. The issue itself says this is latent, not exploitable: the cache uses one ambient identity. The fix needs a migration plus an API cache-key change, so it belongs in its own slice.
• 7: unbounded list reads. ListStoredRecommendations is being reworked by perf(api): page recommendations from an ID-ordered keyset scan instead of the full list #1240 (fix/issue-981, keyset paging). Silently clamping ListActiveSuppressions to 1000 would be unsafe: applySuppressions would stop hiding suppressed recs past the cap. It needs paging or an error, not a truncation.
• 9: CleanupOldExecutions expiry branch. The fix edits the query and the "Branch 3" comment right next to lines that queued branch fix/issue-47 changes (the cancelled/canceled contraction). The right fix is to reuse fix(cleanup): only delete settled executions when their TTL lapses #1173's predicate (a TTL lapse deletes only TerminalExecutionStatuses minus HealthScoredExecutionStatuses). Per that function's own doc, current code never sets expires_at, so the branch only affects legacy rows.
• 10: TryAdvisoryLock overwrite. I could not find a path that reaches it. A pinned *pgxpool.Conn stays acquired until ReleaseAdvisoryLock, so the pool cannot hand the same backend to a second TryAdvisoryLock(lockID) while the map entry exists. Any other session's pg_try_advisory_lock on that key returns false. A failed unlock now closes the connection (fix(server): advisory lock released on the request context strands a task type in permanent already_running #105/chore: minor follow-ups from the #373/#376/#380/#383 reviews #394). So Store never replaces a live entry, and a defensive LoadOrStore would have no fail-before test. I left it alone.

Cross-PR note

#1100 (fix/issue-262) adds an OpenAPI enum: [enabled=false, engine, region, resource_type] for hidden_by. Whichever of #1100 and this PR merges second should add min_count to that enum. Main has no hidden_by schema in openapi.yaml yet, so I could not add it here.

Verification

Unit and mock evidence, plus one real-Postgres test (shared container via testhelpers.RequireMigratedDatabase). All runs used AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true and Go 1.26.9.

Fail-before: I swapped in origin/main's version of the source file under test, with the new tests in place, then restored it.

• Finding 1: go test ./internal/scheduler/ -run 'TestGetRecommendationByID_HiddenByMinCount|TestOverrideHiddenReasons'. On main, HiddenByMinCount failed with expected: []string{"min_count"} actual: []string(nil). AgreesWithFilter failed for every mask with the min_count bit set, including "filter and hidden_by disagree". After the fix, both pass, as does TestOverrideHiddenReasons_MinCountExemptsSavingsPlans. Real Postgres: with main's scheduler sources, the updated TestGlobalFiltersPersistedAmbientAndRegistered/minimum failed with []string(nil) does not contain "min_count". On the branch, go test -tags integration -count=1 -timeout 60m -run 'GlobalFilters|HiddenBy|Override' ./internal/scheduler/ passes.
• Finding 2: on main, TestGlobalConfig_Validate_RIExchangeLimits failed 7 of 9 subtests (all the rejection cases). TestUpdateConfig_RejectsOutOfRangeRIExchangeLimits (internal/api, a router-level PUT /api/config with an execute:ri-exchange principal, so only validation can refuse) failed all 4: on main the negative cap or out-of-range threshold reached SaveGlobalConfig. After the fix, both pass.
• Finding 8: on main, go test -tags integration -count=1 -timeout 60m -run 'TestGetAccountCredential_MissingRowIsNotFoundPostgres' ./internal/config/ failed (real Postgres) with Expected error with "not found" in chain but got nil, and TestPGXMock_GetAccountCredential_NotFound also failed. After the fix, both pass (--- PASS: TestGetAccountCredential_MissingRowIsNotFoundPostgres (30.81s)). The integration test also checks that a stored empty blob still reads back as ("", nil) and a deleted credential as ErrNotFound. No concurrency test: this is a single read with no read-modify-write.
• Finding 11: on main, a scratch test (removed afterwards) calling LoadFromEnv with DB_MAX_CONNECTIONS="100 ", DB_MAX_CONN_LIFETIME=1h30, DB_AUTO_MIGRATE=yes logged main accepted malformed env: MaxConnections=25 MaxConnLifetime=1h0m0s AutoMigrate=false and failed require.Error. After the fix, TestLoadFromEnv/returns_error_naming_every_malformed_variable passes. The TestGetEnvInt/Bool/Duration invalid-value cases now also assert that the error names the variable.

Checks on the branch head:

• go build ./..., go vet ./..., go vet -tags integration ./...: clean
• go test -count=1 on ./internal/scheduler/, ./internal/config/, ./internal/api/, ./internal/database/ and ./internal/mocks/, one package at a time: all ok
• golangci-lint v2.10.1 run --allow-parallel-runners --build-tags integration on scheduler, config, api and database: 0 issues
• gocyclo -over 10 -ignore "_test\.go" on the changed non-test files: no output

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 936996ad = origin/main a14dc8f2 plus these heads, merged in this order:

Branch	Tested head
fix/issue-337	ff3420c
fix/issue-169	7d75eb5
fix/issue-342	66b221f
fix/issue-310	5e11c21
fix/issue-326	5f0ea59
fix/issue-243	365e959
fix/issue-343	6c94036
fix/issue-312	d2848a8
fix/issue-316	acd60dd
fix/issue-339	75ea9b9
fix/issue-339-commitment-type	73bdb19
fix/issue-353	5d25fcd
fix/issue-313	3156bc4
fix/issue-357	53d9e58
fix/issue-368	0ba727d
fix/issue-419	61648c9
fix/issue-111	b04c372
fix/issue-686	0ad90b7
fix/issue-674	793095d
fix/issue-1055	4ec96f1
fix/issue-559	dfa9525
fix/issue-558	034eed4
fix/issue-874	1720539
fix/issue-666	1e120da
fix/issue-668	fb26345
fix/issue-568	ce04173
fix/issue-311	e54f250
fix/issue-177	0cbb88a
fix/issue-151	c21377c
fix/issue-166	41fb77b
fix/issue-27	2d67622
fix/issue-323	69288ae
fix/issue-59	36463e8
fix/issue-43	35e4069
fix/issue-158	6f76f8a
fix/issue-104	bb00491
fix/issue-55	971ef1b
fix/issue-361	89ac220
fix/issue-120	b458719
fix/issue-36	42bac6e
fix/issue-525	57c97dd
fix/issue-712	e5bcda0
fix/issue-72	6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; whichever PR merges second takes that same resolution.

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:

• Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
  • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
  • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
  • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
• What the combined run caught, each fixed on its own branch before this PR opened:
  • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
  • fix/issue-316 rejecting legacy-keyed plans;
  • fix/issue-120 versus main's global-filter test.
• Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• Nothing was applied to any cloud.

Behavior notes for reviewers

• After this PR, PUT /api/config refuses any body while the stored config holds a negative cap. That state was only reachable through PUT /api/config before this change. The 400 names the field, and sending a valid value for it in the same PUT clears it.
• LoadFromEnv now fails boot on a malformed DB_* tuning variable. Every value in the repo's IaC parses. A hand-set additional_env_vars override with a typo would now stop the service at boot instead of running on defaults, which is the intended change.

No frontend change is needed: the UI types hidden_by as string[] and does not map reason strings.

Maintainer: please mirror #120's triage labels on this PR: impact/internal, effort/l, type/chore, triaged, priority/p3, severity/low, urgency/eventually.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE ba558301ffb2371b3e245976db3fbb9892dbcd10

Blocking:

• none

Non-blocking:

• internal/config/validation.go:305: validateRIExchangeLimits runs on the merged whole struct, so a negative cap already stored before this PR makes every later PUT /api/config fail with a 400 (including a kill-switch flip) until the caller also sends a valid value for that field. The PR body states this. It is fine because only the old unvalidated route could have written such a row, but a one-line note in the release notes would help any operator who meets it.
• internal/config/validation.go:332: the NaN/Inf checks can't fire from JSON, since encoding/json can't decode NaN or Inf into a float64. They are harmless defence in depth. The dedicated PUT /api/ri-exchange/config validate() has no NaN check, so "both routes accept the same values" is true in practice only because JSON can't carry those values.
• Scheduler integration subset (-run 'GlobalFilters|HiddenBy|Override'): one of my three runs reported FAIL on this shared VM. I didn't capture the output of that run, and two reruns passed. It is most likely testcontainers contention, but a CI rerun would confirm it isn't a new flake.

Attacked:

• Checked that the list filter and the rec-detail explainer now walk one overrideDimensions table with the same Savings Plans carve-out. Suppressions still run before the override filter on both the list and detail paths, so min_count sees the same post-suppression count in both. applyAccountOverrides is the only caller of filterRecsByResolvedConfigs, and GetRecommendationByID is the only caller of overrideHiddenReasons.
• Checked that PUT /api/config is the only GlobalConfig.Validate caller (handler_config.go:119), and that its bounds match RIExchangeConfigUpdateRequest.validate(). GetAccountCredential has no production callers, so returning ErrNotFound changes no live behavior. Every DB_* value in terraform (AWS Lambda and Fargate, GCP Cloud Run and GKE, Azure AKS and Container Apps, cleanup functions), docker-compose, the Dockerfile and CI still parses: "5432", "8s", tostring(bool).
• Fail-before: I swapped main's scheduler.go, scheduler_overrides.go, validation.go and store_postgres.go into the scratch checkout. Then TestGetRecommendationByID_HiddenByMinCount, TestOverrideHiddenReasons_AgreesWithFilter, TestGlobalConfig_Validate_RIExchangeLimits, TestPGXMock_GetAccountCredential_NotFound and TestUpdateConfig_RejectsOutOfRangeRIExchangeLimits all FAIL. I restored the files afterwards. The database tests don't compile against main's config.go, because they use the new envParser.
• Ran: go build ./...; go vet with and without -tags integration on scheduler, config, api and database; go test -race -short -count=1 on those four packages (ok); -race -tags integration on internal/database (ok), on internal/config -run AccountCredential (ok) and on the scheduler subset above (pass on rerun); golangci-lint on scheduler, config and database (0 issues).

Body-stale:

• none

_{Reviewer: rB6. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at ba558301ffb2371b3e245976db3fbb9892dbcd10 merged onto main (5aa3df5b), together with #1155, #1219, #1221, #1223, #1224, #1225, #1226, #1227, #1256, #1262, #1266, #1270, #1272, #1275, #1283, #1286, #1293, #1297, #1299, #1300, #1301, #1304, #1307, #1311, #1313, #1325 in that order (combined tree f3a3331f, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on ba558301, green CI, and the combined-tree verification above. Squash-merged at exactly ba558301ffb2371b3e245976db3fbb9892dbcd10.