[cristim]

main is red. TestScheduler_ListRecommendations_StaleCacheKicksBackgroundRefresh fails in the Unit and Integration jobs on main (CI run 38887365202 at 18ee477, and every later PR run). The failure:

scheduler_test.go:1003: expected a background refresh for a cache older than the stale TTL

Root cause

Two PRs merged with no textual conflict but broke each other:

• fix(api): only unrestricted readers may kick a recommendations collection #932 (a86c6d7) made ListRecommendations kick a cold-start or stale-cache collection only when the context carries WithBackgroundCollection. The check is mayCollect := BackgroundCollectionAllowed(ctx) in internal/scheduler/scheduler.go; the helpers are in background_collection.go.
• test(scheduler): assert the sentinel-disabled stale refresh never collects #938 (18ee477) was written before fix(api): only unrestricted readers may kick a recommendations collection #932 landed. Its helper staleListRecommendations reads through a plain context.Background(), so no refresh can start.

Effects:

• The positive-control test times out.
• The sentinel test, TestScheduler_ListRecommendations_StaleHoursZeroDisablesBackgroundRefresh, passes vacuously. An unmarked context never refreshes, whatever the TTL.

Fix

Test-only. The helper now reads through WithBackgroundCollection(context.Background()), the same way #932's own tests do. Production code is unchanged. #932's guard is intended: only unrestricted readers may kick a collection.

Verification (macOS)

• Fail before: on origin/main b49b494, go test ./internal/scheduler -run 'TestScheduler_ListRecommendations_Stale' -count=2 -v fails on both runs with the message above.
• Pass at test(scheduler): assert the sentinel-disabled stale refresh never collects #938's own head (7feb381): the stale tests pass, which confirms the conflict.
• Pass after: the same command with -count=20 passes.
• Mutation probes (scratch edits to scheduler.go, reverted):
  • Disable the TTL==0 guard (if false): the sentinel test now FAILS, so it is no longer vacuous.
  • Force mayCollect := true: fix(api): only unrestricted readers may kick a recommendations collection #932's TestListRecommendations_UnmarkedColdStart_NeverMarksOrCollects and TestListRecommendations_UnmarkedStaleCache_NeverRefreshes still fail as before.
• Wider checks: go test ./internal/scheduler ./internal/api/... -count=1 passes, and make lint (go1.26.9 toolchain) reports 0 issues.

The Lint Code failure on main's run is unrelated: golangci-lint timed out fetching its JSON schema over the network.

No issue was filed for this breakage, so the labels are set directly: bug, high severity because it blocks every PR's CI, urgent.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 1 billable file and costs up to $0.25.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 12 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 64 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 4456e160-5f1c-4ed5-af0f-cc4bfd01504c

📥 Commits

Reviewing files that changed from the base of the PR and between b49b494 and d9e192f.

📒 Files selected for processing: 1

• internal/scheduler/scheduler_test.go

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

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

[cristim]

Independent review verdict: CLEAN at d9e192f

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Findings: none, blocking or non-blocking.

What was attacked and how (local macOS, unit tests with mocks):

• Fail-before: with origin/main's scheduler_test.go swapped in, TestScheduler_ListRecommendations_StaleCacheKicksBackgroundRefresh FAILS at scheduler_test.go:1003 with "expected a background refresh for a cache older than the stale TTL". The sentinel test passes there only vacuously. The file was restored and the worktree is clean.
• At head:
  • go test ./internal/scheduler -run Stale -count=20: 60/60 passed.
  • go test -race ./internal/scheduler -count=1: 196 passed, 0 SKIPs.
• Mutation probes on scheduler.go (each restored exactly):
  • Disabling the TTL==0 guard makes StaleHoursZeroDisablesBackgroundRefresh FAIL, so the sentinel now has teeth.
  • Forcing mayCollect := false makes StaleCacheKicksBackgroundRefresh FAIL.
• Production sanity:
  • The only non-test producer of WithBackgroundCollection is recommendationsReadContext (internal/api/handler_recommendations_refresh.go:141), and only when scope.AllowsAll().
  • All three production ListRecommendations callers go through it (handler_dashboard.go:38, handler_inventory.go:244, handler_recommendations.go:105).
  • So fix(api): only unrestricted readers may kick a recommendations collection #932's guard is the intended behavior, and the test now matches it. Production code is unchanged.

Gate: labels are set directly (no linked issue, as the body says). CodeRabbit is green. Merge waits on CI being green at this SHA.

[cristim]

Independent review of d9e192f: CLEAN

The fix is correct, test-only and in the right place. Two pre-existing vacuous Lambda tests (same family, not caused by this PR) are filed as non-blocking follow-ups.

Findings

1. Non-blocking, low, pre-existing: internal/scheduler/scheduler_test.go:1060 (TestScheduler_ListRecommendations_LambdaSkipsBackgroundRefresh, ctx at :1061) and :1118 (TestScheduler_ListRecommendations_ColdStart_Lambda_NeverMarksOrCollects, ctx at :1119) use a plain context.Background(). With fix(api): only unrestricted readers may kick a recommendations collection #932's guard (scheduler.go:1153) an unmarked read never collects, so these pass whether or not the Lambda guard exists. Mutation: changing scheduler.go:1183 from if mayCollect && !s.isLambda { to if mayCollect { leaves both tests PASS. Same bug class as the one this PR fixes, so a follow-up should mark their ctx with WithBackgroundCollection. I did not touch them. I did not probe the isLambda guards at scheduler.go:1452 and :1502.
2. Informational: the other plain-ctx ListRecommendations callers in tests (scheduler_suppressions_test.go, scheduler_overrides_test.go, scheduler_global_filters_integration_test.go, scheduler_strict_list_test.go:73, scheduler_test.go:936/974/1053) assert read-path filtering only, not collection. Not vacuous.

What I attacked

• Setup: git rev-parse HEAD in my worktree is d9e192f; its parent is b49b494, the PR base.
• Root cause: internal/scheduler/scheduler.go:1153 sets mayCollect := BackgroundCollectionAllowed(ctx). It gates the cold-start kick (:1154) and the stale refresh (:1183). background_collection.go:11-30 documents it as fail-closed, so a caller scoped to a subset of accounts cannot trigger an all-accounts collection (sec(api): scoped readers of recommendations can trigger a global collection and take the in-flight marker (non-Lambda cold-start/background refresh) #627). fix(api): only unrestricted readers may kick a recommendations collection #932's body confirms this is a decided design. The fix belongs in the test helper, and production code is unchanged.
• Fail-before: restoring the pre-fix scheduler_test.go and running go test ./internal/scheduler -run 'TestScheduler_ListRecommendations_Stale' -count=2 -v. StaleCacheKicksBackgroundRefresh FAILS both runs (5.00s timeout, scheduler_test.go:1003: expected a background refresh...). The sentinel passes vacuously, as the PR claims.
• Pass-after: the same command with -count=20 at the PR head. All 60 results pass (20 each for StaleCacheKicks, StaleHoursZero and StaleSingleFlight).
• Sentinel no longer vacuous: changing if disabled { to if false && disabled { in scheduler.go makes StaleHoursZeroDisablesBackgroundRefresh FAIL (scheduler_test.go:992: background refresh started although ...). Reverted.
• fix(api): only unrestricted readers may kick a recommendations collection #932 guard still tested: forcing mayCollect := true makes TestListRecommendations_UnmarkedColdStart_NeverMarksOrCollects and TestListRecommendations_UnmarkedStaleCache_NeverRefreshes FAIL. Reverted, and git status is clean.
• Callers audit: grepped .ListRecommendations( in *_test.go under internal/. See findings 1 and 2.
• Wider checks: go test ./internal/scheduler ./internal/api/... -count=1 gives 3534 passed. GOTOOLCHAIN=go1.26.9 make lint gives 0 issues.
• Not covered: CI status on this SHA, and CodeRabbit. This is not an approval or a merge.

[cristim]

Gate record for d9e192f:

• Independent reviews: two non-author reviewers each found it CLEAN at this SHA. One is from claude-cc-reviewer; the other is at test(scheduler): kick the stale refresh with an unrestricted reader #960 (comment).
• CI: green at this SHA (pre-commit, then CI - Build & Test). mergeStateStatus is CLEAN.
• Local verification on macOS, described in the PR body and reproduced by the reviewers:
  • fail-before on main;
  • 20 of 20 passes after the fix;
  • mutation probes showing the sentinel test can now fail.
• Follow-up from the review: two Lambda no-refresh tests are vacuous for the same reason, filed as test(scheduler): Lambda no-refresh tests pass vacuously since #932 #962. They are not blocking.

This is ready for a human merge. Merging it turns main green again, and it is test-only, so there is no deploy.