[IAmJSD]

Summary

Three items from #111, re-verified against main at 7b86e386 before changing anything.

1. Suppression-read failure no longer over-shows (item 5, MEDIUM)

Root cause. ListRecommendations and GetRecommendationByID (internal/scheduler/scheduler.go) and loadSuppressionIndex (used by ScanRecommendations, internal/scheduler/scheduler_scan.go) logged a ListActiveSuppressions error and continued with the un-suppressed list. Suppressions are the duplicate-purchase guard. On any DB blip, every just-bought commitment came back at its full pre-purchase Count with no "recently purchased" badge, so it looked buyable again.

Fix. All three now return the error (fail closed), as ListRecommendationsStrict already did. applySuppressions also returns no recs on error, so a future caller that drops the error still shows nothing rather than the un-suppressed list. ScanRecommendations fails before it reads any chunk or calls fn. HTTP callers already turn the error into a 500. The inventory coverage view (fetchCoverageOnDemandRecs) already treats a failed read as "no uncovered gap", which errs toward showing less.

Unchanged on purpose. Account-override read failures still over-show. Overrides are a display filter, not a spend guard, and the issue item covers only suppressions. The comments that cited applySuppressions as the over-show precedent are updated: scheduler_overrides.go, config/recommendation_overrides.go, handler_dashboard.go, the ListRecommendationsStrict doc and the ScanRecommendations doc. ScanRecommendations' chunk loop moved into a scanChunks helper to stay under the gocyclo limit of 10. That move changes no behavior.

2. Grace-period config read failure fails the purchase (A01-016, LOW)

Root cause. Handler.gracePeriodConfig (internal/api/handler_purchases.go) swallowed the GetGlobalConfig error and returned nil. BuildSuppressions then used the 7-day default. During a DB blip this wrote suppression rows for a provider the operator had switched off (grace 0), or with the wrong expiry for the others.

Fix. gracePeriodConfig now returns (cfg, error). Both call sites, executePurchase and persistRetryExecution, return that error before anything is persisted, as internal/purchase (AttachResolvedRecommendations) already does. BuildSuppressions' own nil-config fallback is unchanged, since no caller passes nil any more (see "Out of scope").

3. IsLambda doc wording (item 8, wording only)

The internal/runtime/runtime.go doc said AWS_LAMBDA_RUNTIME_API is "absent on container images". Lambda container images do set it. The doc now says it is absent on non-Lambda deployments (local dev, Fargate, Cloud Run, Container Apps). It also says the flag only answers whether the process is frozen between invocations, and must not be used for single-instance or shared-state questions.

Refs #111

Verification

All evidence is unit/mock/fixture evidence: in-memory fakes and testify mocks, no database. Environment: AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.9.

Fail-before: the new and changed tests were run on the unfixed code at 7b86e386:

go test ./internal/scheduler/ -count=1 -v -run 'SuppressionReadFailure|ReadErrors|ApplySuppressions_ReadFailure|OverrideReadFailureOverShows'
--- PASS: TestScanRecommendations_OverrideReadFailureOverShows
--- FAIL: TestScanRecommendations_SuppressionReadFailureFailsClosed   (fn must not run when the suppression read fails)
--- FAIL: TestListRecommendationsStrictReturnsReadErrors/suppressions (lenient path: expected error, got nil)
--- FAIL: TestListRecommendations_SuppressionReadFailureFailsClosed   (expected error, got nil)
--- FAIL: TestGetRecommendationByID_SuppressionReadFailureFailsClosed (expected error, got nil)
--- FAIL: TestApplySuppressions_ReadFailureReturnsNoRecs              (got the un-suppressed rec)

go test ./internal/api/ -count=1 -v -run 'GlobalConfigErrorFailsBeforePersist'
--- FAIL: TestHandler_executePurchase_GlobalConfigErrorFailsBeforePersist (an error is expected but got nil)
--- FAIL: TestHandler_retryPurchase_GlobalConfigErrorFailsBeforePersist   (an error is expected but got nil)

Pass-after: the same commands on this branch pass all of the above. The api tests also assert that SavePurchaseExecution, LinkRetryExecutionAtomic and CreateSuppressionTx are never called, and that the error is a server error, not a 4xx.

Other checks on this branch:

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

No integration test was added. The change is error propagation after an existing store read, with no SQL, schema or write-path change and no new concurrency. Real Postgres would only reproduce the failed read that the fakes inject. No migration.

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

Branch	Tested head
fix/issue-243-rest	007c163
fix/issue-1206	312072f
fix/issue-111-email	6d422e2
fix/issue-111-fail-closed	3817e29
fix/issue-712-age-cap	a91120b
fix/issue-712-fargate	aeaf0d4
fix/issue-613-legacy-ids	350e837
fix/issue-120-guards	6f38ebc
fix/issue-120-shared-predicate	cb25735
fix/issue-151-account-update-keep	d20cf01
fix/issue-713-upcoming-filter	e73fdfb
fix/issue-263-bastion-opts	7861f32
fix/issue-43-fixture-ids	8cee203
fix/issue-177-makefile-walk	365a9d6
fix/issue-27-seller-not-registered	d845f66
fix/issue-313-user-columns	6d759dd
fix/issue-353-mock-panics	aac2edf
fix/issue-8-config-stamps	b548ae5
fix/issue-30-monthly-totals	f637921

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. The full suite ran on f7c48020, which differs from this tree only by test-only commits on fix/issue-30, fix/issue-713 and fix/issue-263 that replace placeholder account-number literals with named constants:

• 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, every changed package that has them, one package at a time (-tags integration -count=1): internal/analytics, internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase, internal/scheduler and internal/server all pass. Migrations 000149, 000150 and 000151 apply in order on this tree.
• Frontend: the changed jest files (approval-details, settings-accounts, api) pass (221 tests).
• Secrets: the first run flagged 9 new fixture lines (placeholder account numbers next to an account key); fixed with named constants. Re-run on this tree: 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: 183 PASS, 0 FAIL.
• Terraform .tftest.hcl files were not run locally (CI runs them). Nothing was applied to any cloud.

Behavior change to note

While purchase_suppressions cannot be read, GET /api/recommendations (list, paged and detail) and the dashboard now answer 500 instead of an un-suppressed list. This is the "cannot buy" failure direction the issue asks for. The Recommendations page already shows its "Failed to load recommendations" state for this.

Out of scope (left for later)

• The other open items in chore(server/scheduler): medium/low findings from the 2026-07-28 full review #111: the federation .tfvars escaping, the IsLambda single-instance consumer at internal/server/app.go and a separate distribution discriminator, and all internal/email items (9-12, and A06-005/012/014). Another agent has the email items.
• Making config.BuildSuppressions reject a nil config, which the A01-016 note also suggests. Every caller now passes a non-nil config or fails first, so the nil fallback in BuildSuppressions is unreachable from these paths. It was left as is to keep this change minimal.

Frontend follow-up for the UI owner

None required. Optional: the Recommendations page could show a more specific message when the failure is a suppression read ("purchase history temporarily unavailable"). That would need a distinguishable error code from the API, which this PR does not add.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p3, severity/low, urgency/eventually, impact/many, effort/l, type/bug, triaged.

🤖 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 58 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: 7b86e38
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: fdf26888-fd8f-416c-b68d-6fa6f40953d9

Commits

Reviewing files that changed from the base of the PR and between 7b86e38 and 7165382.

Files ignored due to path filters: 1

• go.sum is excluded by !**/*.sum

Files selected for processing: 11

• go.mod
• internal/api/handler_dashboard.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_grace_config_test.go
• internal/config/recommendation_overrides.go
• internal/runtime/runtime.go
• internal/scheduler/scheduler.go
• internal/scheduler/scheduler_overrides.go
• internal/scheduler/scheduler_scan.go
• internal/scheduler/scheduler_scan_test.go
• internal/scheduler/scheduler_strict_list_test.go

• 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 7165382cadc46c4d9beaf86df78e04d0ba203be4

Blocking:

• none

Non-blocking:

• The change reaches further than the recs page. getDashboardSummary (internal/api/handler_dashboard.go) also calls ListRecommendations, so the dashboard summary now returns 500 when the suppression read fails, where it used to show over-counted savings. The inventory coverage view swallows the error (handler_inventory.go, returns nil), so it under-shows. Both are the safe direction and fit the issue's "cannot buy rather than buy twice". Worth a line in the PR body so nobody is surprised by a dashboard 500 during a DB blip.
• When the suppression read fails, ScanRecommendations and ListRecommendations now return before finishCacheRead, so no stale-while-revalidate kick happens on that request. This matches every other error return in those functions and doesn't matter in practice.

Attacked:

• Callers: listed every caller of ListRecommendations, GetRecommendationByID, ScanRecommendations and BuildSuppressions. The two gracePeriodConfig call sites now fail before persistExecutionAndSuppressions / persistRetrySuccessorTx. Before that point nothing is written: no approval email, no idempotency row. internal/purchase's two BuildSuppressions callers already fail on a config read error. The ScanRecommendations refactor into scanChunks keeps the same loop and the same exits. The suppression read happens before any chunk read, so nothing has streamed when it fails.
• Fail-before: checked out the merge-base versions of scheduler.go, scheduler_scan.go and handler_purchases.go and kept the new tests. All 5 scheduler tests and both api GlobalConfigErrorFailsBeforePersist tests failed, exactly as the body lists. Restored afterwards. The api tests assert that SavePurchaseExecution, LinkRetryExecutionAtomic and CreateSuppressionTx are never called, and that the error is not a client error.
• Commands: go build ./...; go vet with and without -tags integration on scheduler, api and config; go test -race -short -count=1 on scheduler, api, config and runtime (ok). go test -race -tags integration -count=1 on the whole internal/scheduler package (ok) and on internal/api -run 'Suppress|Retry|ExecutePurchase|Recommendation|Grace' (ok).
• Scope: Refs #111 is right. The PR does three of the issue's many items.

Head moved: the brief named 3817e29e. The new head 7165382c adds one commit, fix(deps): bump golang.org/x/net to v0.62.0. Its go.mod/go.sum change is byte-identical to #1564, so the two merge cleanly in either order. Re-checked on 7165382c: go mod verify passes, go build ./... and go vet pass, and go test -race -short -count=1 on scheduler, api and config is ok. The rest of this review was done on 3817e29e, whose code is unchanged at this head.

Body-stale: the body doesn't mention the x/net bump (non-blocking; it fixes the "Build Docker Image" govulncheck failure that every PR currently hits).

_{Reviewer: rG4. 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]

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

Blocking:

• none

Non-blocking:

• internal/api/handler_inventory.go:252 (unchanged): the coverage view still swallows the now-failing ListRecommendations and treats it as "no uncovered gap". This errs toward showing less, as the body says, but the coverage page now silently under-reports on-demand gaps during a suppression-read blip. A log line there would make that visible.
• The A01-016 note also suggests that config.BuildSuppressions should reject a nil config. The body explains why that is left out, and both API call sites now fail before reaching it. Worth a follow-up so a future caller can't bring the 7-day fallback back.

Attacked:

• Checked every caller of ListRecommendations, GetRecommendationByID, ScanRecommendations and ListActiveSuppressions. The only non-test readers are the scheduler's three paths plus ListRecommendationsStrict, so nothing still fails open. The scan now fails before fn, so a streamed response can't be half-written. finishCacheRead is skipped on error, which is harmless.
• Checked both gracePeriodConfig sites. Retry fails after requireAttributablePurchaseCreator and before persistRetrySuccessorTx. Execute fails before persistExecutionAndSuppressions. No row, linkage or suppression is written in either case, so there is no partial state.
• Fail-before: I reverted handler_purchases.go, scheduler.go and scheduler_scan.go to the merge-base. All 5 new or changed scheduler tests and both api GlobalConfigErrorFailsBeforePersist tests then FAIL. They pass once the fix is restored.
• Ran:
  • go build ./...
  • go vet on the changed packages, with and without -tags integration
  • go test -race -short -count=1 on scheduler, api and runtime: ok
  • go test -race -tags integration -count=1 on scheduler and server: ok
  • golangci-lint v2.10.1, with and without --build-tags integration: 0 issues
  • gocyclo -over 10 on the changed files: clean

Body-stale: none

_{Reviewer: rH2. 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 7165382cadc46c4d9beaf86df78e04d0ba203be4 merged onto main (bbf0a839), together with #1552, #1553, #1551, #1246, #1548, #1549, #1554, #1560 in that order (combined tree 7f9817d4, 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 2 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: 2 approving independent review(s) on 7165382c (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 7165382cadc46c4d9beaf86df78e04d0ba203be4.