[IAmJSD]

Summary

This is the age-based sweep from #120 (the "unbounded list reads" item's fix direction, listed as "Left (workable)" in the 2026-10-28 status update). The other open items of #120 are not touched.

Root cause

UpsertRecommendations only evicts rows of the (provider, account_key) pairs that collected successfully in the current run. That's on purpose: a failed pair keeps showing its previous rows instead of going blank. But nothing ever removed those rows later. If a pair never collects again, its rows stay on the Recommendations page and in ListStoredRecommendations forever. That happens when a provider is removed from enabled_providers, an account is disabled, or credentials keep failing. Registered accounts that are deleted outright were already covered, because recommendations.cloud_account_id is ON DELETE CASCADE.

Fix

• PostgresStore.PruneStaleRecommendations(ctx, retentionDays) (int64, error) (internal/config/store_postgres_recommendations_prune.go, added to config.StoreInterface and mocks.MockConfigStore):
  • Deletes the rows of every (provider, account_key) pair that has no row with collected_at in the last retentionDays days.
  • A pair with even one row inside the window keeps all its rows, however old the others are. A partial collection leaves rows for its failed services behind on purpose.
  • A pair's newest row tells us when it last collected. Every collection stamps the rows it upserts with its own collected_at, and a successful collection also evicts the pair's older rows, so no separate bookkeeping table or migration is needed.
  • It rejects retentionDays <= 0 with an error.
• Concurrency: the prune runs in its own transaction and first runs LOCK TABLE recommendations IN SHARE ROW EXCLUSIVE MODE.
  • That mode conflicts with the ROW EXCLUSIVE lock that every INSERT/UPDATE/DELETE takes, and with itself. So the prune waits for any in-flight UpsertRecommendations, and collections that start meanwhile wait for the prune, which is a single DELETE.
  • The DELETE's snapshot is taken after the lock is granted. So a collection that committed fresh rows for a pair always counts, and no collection can commit between the pair check and the delete.
  • Readers (ACCESS SHARE) are not blocked.
  • The prune holds no other lock before requesting this one, and nothing in the codebase row-locks recommendations without writing to it, so the wait cannot deadlock.
  • The row-level collected_at < cutoff bound follows from the NOT EXISTS, but I kept it so the planner can drive the delete from recommendations_collected_idx. It is the one clause no mutation probe can kill; see Verification.
• Where it runs: at the end of Scheduler.persistCollection, best-effort, after a successful UpsertRecommendations (a failed write skips it).
  • I piggybacked on the collection run rather than the cleanup sweep. Collection runs daily on every cloud by default since fix(iac): fire delayed purchases by default; collect AWS recommendations at 02:00 UTC #1580, while enable_cleanup_schedule is opt-in (default false) on every platform. A sweep there would never run on most deployments.
  • A prune error is logged and never fails the collection or touches the error/partial banner.
• Config: CUDLY_STALE_RECOMMENDATION_RETENTION_DAYS, default config.DefaultStaleRecommendationRetentionDays = 30.
  • It is read on every collection. A value that is not a positive whole number logs a warning and falls back to the default, so a typo can't become "prune everything".
  • Why 30: collection is daily at 02:00 UTC on every cloud (fix(iac): fire delayed purchases by default; collect AWS recommendations at 02:00 UTC #1580), so 30 days means about 30 missed collections in a row. That rides out an outage or an expired credential nobody fixes for a few weeks, and a pair that is gone for good still disappears within a month. It also matches DefaultExecutionTTLDays.
  • It is documented in specs/recommendations-cache.md and .env.example. It is not exposed as a Terraform variable; the default applies unless an operator sets the env var on the compute.

Interaction with suppressions and purchases

• Suppressions (purchase_suppressions) are keyed by (execution_id, account_id, provider, service, region, resource_type, engine) with their own expires_at. They have no FK to and no id reference into recommendations. The scheduler subtracts them from stored recs at read time. Pruning a pair's rows leaves its suppressions untouched: they have nothing to subtract from until they expire on their own. If the pair collects again, any still-active suppression applies to the new rows as before.
• Existing purchases (purchase_executions.recommendations) store a JSONB snapshot of the recs at creation time. They have no FK to recommendations.id and nothing reads them back from the cache, so pruning never changes, orphans or blocks an existing execution, approval or fire.
• New purchases / plan steps look up the stored row by identity tuple (purchase_pricing.go, planstep/resolve.go). After a prune they get the same result as after a normal eviction: the existing fail-closed 409 "is not in the current recommendation set; refresh the recommendations and try again", or the rec is simply absent from the step. The prune adds no new failure mode.
• If a pruned pair collects again, its recs come back with the same application id (payload->>'id', derived from the identity tuple). The table's UUID primary key is new, because rows are re-inserted, not updated.

Behavior a reviewer should weigh

• If every collection fails for 30+ days, all rows are pruned and the page empties, while the existing error banner keeps showing why. I think that is correct: those recs are priced off usage at least 30 days old and shouldn't be offered for purchase. But it is a judgment call.
• Old rows inside a pair that is still collecting (left behind by partial collections) are still never pruned. That is deliberate per the scope ("never touch rows of a pair that collected recently").

Refs #120

Verification

All commands ran with export PATH=$HOME/.local/go/bin:$HOME/go/bin:$HOME/.local/bin:$PATH AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true on the branch head.

Build/vet/lint:

• go build ./...: ok
• go vet ./... and go vet -tags integration ./...: ok
• golangci-lint run --allow-parallel-runners ./internal/config/... ./internal/scheduler/... ./internal/mocks/...: 0 issues (also with --build-tags integration)
• gocyclo -over 10 -ignore "_test\.go" on the changed non-test files: nothing over 10
• pre-commit run {trailing-whitespace,end-of-file-fixer,markdownlint,gosec,go-fmt} --files <changed files>: passed

Unit tests (mock evidence): go test -count=1 ./internal/scheduler/, ./internal/config/ and ./internal/mocks/: ok. New tests in internal/scheduler/stale_recommendations_test.go:

• env parsing: unset, valid, padded, 0, negative, 720h and garbage values
• the prune runs with the default and with the env value
• a prune error stays best-effort
• an upsert failure skips the prune

Integration tests (real Postgres via testcontainers, internal/config/store_postgres_recommendations_prune_integration_test.go):

go test -tags integration -count=1 -timeout 20m -run 'TestPostgresStore_PruneStaleRecommendations' ./internal/config/
--- PASS: TestPostgresStore_PruneStaleRecommendations_PairScope
--- PASS: TestPostgresStore_PruneStaleRecommendations_RejectsNonPositiveRetention
--- PASS: TestPostgresStore_PruneStaleRecommendations_ConcurrentCollection
--- PASS: TestPostgresStore_PruneStaleRecommendations_RacingCollections

What each test covers:

• PairScope: a stale registered pair is pruned. A fresh pair with mixed ages (1h, 31d and 40d old rows) keeps every row. A pair whose newest row is 29 days old is kept. A stale ambient aws pair is pruned even though a fresh ambient azure pair exists, so provider is part of the pair key. A stale registered azure pair is pruned despite the fresh ambient azure pair, so the account is part of the key. The delete count is exact and a second run deletes nothing.
• RejectsNonPositiveRetention: 0 and -1 return an error and delete nothing.
• ConcurrentCollection:
  • Setup: a real UpsertRecommendations refreshes one row of a stale pair and is held open. A test transaction holds the recommendations_state row lock, so the upsert stops after it has upserted the row. The real prune is then started and the test waits until it is blocked.
  • After release: both rows of the refreshed pair survive (the refreshed row with the collection's timestamp, and the untouched old row). The uncollected pair's row is pruned.
• RacingCollections: 20 rounds of an unordered race between a successful collection refreshing a stale pair and a prune. The refreshed row survives every round.

Fail-before / mutation evidence (integration tests on real Postgres; each mutation was applied to the branch and reverted afterwards):

Mutation	Result
Prune is a no-op (main's behavior: nothing ever deletes these rows)	PairScope and ConcurrentCollection FAIL
Scheduler without the persistCollection hook (main's behavior)	3 scheduler unit tests FAIL
LOCK TABLE removed	ConcurrentCollection FAILS: pair-untouched deleted (prune judged the pair on a pre-commit snapshot)
fresh.provider = r.provider removed	PairScope FAILS (the fresh azure ambient pair shields the stale aws ambient pair)
fresh.account_key = r.account_key removed	PairScope and ConcurrentCollection FAIL
NOT EXISTS (pair scope) removed	PairScope FAILS (fresh-old and fresh-older deleted) and ConcurrentCollection FAILS
Lock and row-level bound both removed	ConcurrentCollection FAILS (the just-refreshed row is deleted) and RacingCollections FAILS

Removing only the row-level collected_at < cutoff bound while keeping the lock survives, as expected: under the lock it follows from the NOT EXISTS and is kept for the index plan, as described above.

I did not run whole integration packages, -race ./... or git-secrets; those run on the combined tree.

Combined-tree verification. Tested on a local, never-pushed tree: queue/platform @ 594ac3ec = origin/main ae6854b3 plus:

Branch	Tested head
fix/issue-357-delete-cleanup-lambda	56989d8
fix/issue-36-archera-gate	2d22986
fix/issue-35-assert-api-a	f793689
fix/issue-35-assert-api-b	fac72f3
fix/issue-120-stale-rec-sweep	07c2e9c
fix/issue-981-summary-sql	7056746

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: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests (-tags integration -count=1, one package at a time): internal/api, internal/auth, internal/config, internal/scheduler, internal/server and internal/testutil pass.
• Secrets: git-secrets on the changed files found nothing, and a planted fake key was flagged; scripts/test-git-secrets-allowlist.sh passes.

Out of scope (left for owner decisions, per #120)

• (a) the 0 to 95 RI-exchange utilization-threshold coercion
• (b) external_id changes orphaning history
• (c) clamping ListStoredRecommendations, which waits on the feat(recommendations): bound GET /api/recommendations with pagination and slimmer rows #777 paging decision

I also didn't touch rows that partial collections leave behind inside a pair that is still collecting.

Frontend follow-up for the UI owner

None required. Pruned rows just stop appearing, and the existing freshness/error banner is unchanged.

Labels

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]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: ae6854b
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: d3d0a230-33fb-480d-a356-0c309037fa69

Commits

Reviewing files that changed from the base of the PR and between ae6854b and 07c2e9c.

Files selected for processing: 11

• .env.example
• internal/config/constants.go
• internal/config/interfaces.go
• internal/config/recommendations_scoped_delete_guard_test.go
• internal/config/store_postgres_recommendations_prune.go
• internal/config/store_postgres_recommendations_prune_integration_test.go
• internal/mocks/stores.go
• internal/scheduler/scheduler.go
• internal/scheduler/stale_recommendations.go
• internal/scheduler/stale_recommendations_test.go
• specs/recommendations-cache.md

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{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 07c2e9c29c18bf76c5f0f3173de2301d59128c12

Blocking:

• none

Non-blocking:

• Lock wait: LOCK TABLE recommendations IN SHARE ROW EXCLUSIVE MODE has no lock_timeout. While the prune is queued behind a long writer (a big collection's batched upsert, or a DeleteCloudAccount cascade), every new writer to recommendations queues behind it too. Readers are not affected because they take ACCESS SHARE. In practice upserts are short, so this is acceptable. A SET LOCAL lock_timeout (a few seconds) would make the best-effort prune give up rather than stall the next collection's writes. Optional.
• CUDLY_STALE_RECOMMENDATION_RETENTION_DAYS is not exposed as a Terraform variable, so on Lambda, Fargate, Cloud Run, Container Apps and AKS an operator can only change it by editing the compute env out of band. The default of 30 is sane. The body states this, so it is a product call, not a defect.
• The "every collection fails for 30+ days and the page empties" behaviour (raised in the body) is a product decision for the acting owner. I agree with the author's reasoning: those recommendations are priced off usage at least 30 days old.

Attacked:

• Pair-key consistency: the prune keys on (provider, account_key), the same key UpsertRecommendations' eviction uses. account_key is the generated COALESCE(cloud_account_id, nil-uuid) from 000030. insertRecommendationsBatch's ON CONFLICT ... DO UPDATE always rewrites collected_at, so a pair that collected has fresh rows, and a successful collection with zero recs leaves no rows to prune.

• Nothing has an FK to recommendations (grepped the migrations for REFERENCES recommendations), so the delete can't cascade into purchases or plans.

• Deadlocks: the prune holds no lock before its table lock, and deleting a referencing row fires no FK trigger toward cloud_accounts. So there is no cycle with the DeleteCloudAccount cascade or with recommendations_state.

• Fail-before and mutations, each applied in the scratch tree and reverted:

  • Removing the persistCollection hook (main's behaviour): 3 scheduler tests fail.
  • Moving the prune into the upsert-error branch: UpsertFailureSkipsPrune fails. The repo's mock shadows AssertNotCalled over its own call log, so the zero-arg form is not vacuous.
  • Replacing LOCK TABLE with SELECT 1: ConcurrentCollection fails ("only the uncollected pair's row is pruned").

• Checks run:

  • go build ./..., and go vet with and without -tags integration
  • go test -race -short -count=1 on internal/scheduler, internal/config and internal/mocks
  • go test -race -tags integration on the four PruneStaleRecommendations tests plus TestNoUnscopedRecommendationsDelete, and on all of internal/scheduler
  • gocyclo -over 10 on the changed non-test files

  All pass and gocyclo is silent. I did not run golangci-lint locally: two attempts on this shared VM never finished, so the CI lint job is the check.

• Scope: this is the "age-based sweep" half of chore(config/db): medium/low findings from the 2026-07-28 full review #120's unbounded-reads item, and Refs #120 is correct, because the clamp waits on feat(recommendations): bound GET /api/recommendations with pagination and slimmer rows #777 and other items are open.

Body-stale:

• none

_{Reviewer: rQ1. 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 07c2e9c29c18bf76c5f0f3173de2301d59128c12 merged onto main (d71af683), together with #1591, #1595, #1596 in that order (combined tree 71d6c3d4, 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 07c2e9c2, green CI, and the combined-tree verification above. Squash-merged at exactly 07c2e9c29c18bf76c5f0f3173de2301d59128c12.