[IAmJSD]

What was vacuous

Since #932, Scheduler.ListRecommendations kicks the cold-start and stale-cache collections only when the ctx carries WithBackgroundCollection (mayCollect := BackgroundCollectionAllowed(ctx)). Two Lambda tests still read with a plain context.Background():

• TestScheduler_ListRecommendations_LambdaSkipsBackgroundRefresh
• TestScheduler_ListRecommendations_ColdStart_Lambda_NeverMarksOrCollects

With an unmarked ctx, mayCollect is false, so nothing is ever collected and both tests pass whether or not the isLambda guards exist. The probes below show this on the origin/main test files.

Change (test-only, no production code touched)

internal/scheduler/scheduler_test.go, both tests edited in place:

• Both now read with WithBackgroundCollection(context.Background()), so the isLambda guard is the only thing that stops a collect. A require.True(BackgroundCollectionAllowed(ctx)) precondition makes the test fail if someone drops the marker again.
• The first store call each guard skips is stubbed with .Maybe(): GetGlobalConfig (TTL resolution) for the stale test, and MarkCollectionStarted returning "lost the race" for the cold-start test. A removed guard then fails the existing AssertNotCalled with a clear message instead of an unexpected-call panic.

internal/scheduler/background_collection_test.go, one new test:

• TestMaybeKickBackgroundRefresh_LambdaGuard. The issue asked me to check the guards at scheduler.go:~1452 and ~1502. The ~1452 guard (kickColdStartCollection) is now covered by the cold-start test above. The ~1502 guard (maybeKickBackgroundRefresh) can't be reached through ListRecommendations, because the outer mayCollect && !s.isLambda check returns first, and no test covered it. So the new test calls maybeKickBackgroundRefresh directly. The refresh's first store call blocks until cleanup, so after a wrong kick the collecting flag (set synchronously by the CAS) is still held when the test checks it. The test doesn't depend on timing to catch a removed guard. A non-Lambda subtest on the same harness is the positive control and must see the refresh start.

The new test went into the small background_collection_test.go rather than the 2.8k-line scheduler_test.go. Nothing was moved, and no existing test covers the same thing.

Closes #962

Verification

All commands were run locally on macOS from the worktree with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. Nothing calls a cloud API, purchases anything, sends email or deploys.

Mutation probes (unit evidence; production code mutated, run, then reverted with git checkout, tree confirmed clean)

Run against head 83459e4c with go test ./internal/scheduler/ -count=1 -run 'LambdaSkipsBackgroundRefresh|ColdStart_Lambda_NeverMarksOrCollects|TestMaybeKickBackgroundRefresh_LambdaGuard' -v, and the same run with the origin/main versions of the two test files restored (fail-before).

Guard removed	Mutation	origin/main tests	This PR
G1 ListRecommendations scheduler.go:1183	if mayCollect && !s.isLambda { -> if mayCollect {	both PASS (vacuous)	LambdaSkipsBackgroundRefresh FAIL (GetGlobalConfig was called 1 time(s) ... expected none). ColdStart_Lambda_NeverMarksOrCollects also FAILs (same message).
G2 kickColdStartCollection scheduler.go:1452	if s.isLambda { -> if false {	both PASS (vacuous)	ColdStart_Lambda_NeverMarksOrCollects FAIL (MarkCollectionStarted was called 1 time(s) ... expected none)
G3 maybeKickBackgroundRefresh scheduler.go:1502	if s.isLambda { -> if false {	both PASS (no test reached it)	TestMaybeKickBackgroundRefresh_LambdaGuard/lambda_never_kicks FAIL (collecting flag set; refresh started on Lambda). The control subtest still PASSes.

With no mutation, all three tests PASS. Each guard has at least one test that fails when that guard alone is removed.

Suite, lint and hooks

• go build ./...: OK
• go vet ./...: OK
• go test ./internal/scheduler/... -count=1: ok
• go test -race ./internal/scheduler/... -count=1: ok
• Flake check: go test -race ./internal/scheduler/ -count=100 -run '<the three tests>': ok
• golangci-lint run --allow-parallel-runners ./internal/scheduler/... (v2.10.1): 0 issues
• scripts/gofmt-hook.sh, make tidy-check, scripts/gosec-hook.sh (gosec v2.28.0) on the changed files: all exit 0
• git-secrets --scan on both changed files with --register-aws patterns (as in CI) plus the repo's .gitallowed: clean. The diff adds no 12-digit literals.
• gocyclo only looks at non-test files, and none changed.

Known-red test on main

When this branch was cut (b49b494e), main was red on TestScheduler_ListRecommendations_StaleCacheKicksBackgroundRefresh. I confirmed it fails on a clean b49b494e and left it alone. #960 has since merged (2ef2f5d2) and this branch is rebased onto it, so the whole scheduler package is green at this head.

Overlap with #960 / #965

Both of those PRs edit only the staleListRecommendations helper (scheduler_test.go ~945). This PR's hunks start at ~1055, and the helper is untouched.

• Before rebasing, on base b49b494e:
  • git merge-tree --write-tree fix/issue-962 origin/fix/scheduler-stale-refresh-test gave exit 0, clean (e5a050d9).
  • The same against origin/fix/scheduler-stale-kick-test-context gave exit 0, clean (c5faeebb).
  • Trial merges with each passed go test -race ./internal/scheduler/....
• After rebasing onto main with test(scheduler): kick the stale refresh with an unrestricted reader #960 merged:
  • Against origin/fix/scheduler-stale-refresh-test: exit 0, clean.
  • Against origin/fix/scheduler-stale-kick-test-context: exit 1, conflict in scheduler_test.go at the staleListRecommendations ctx := line only. git merge-tree origin/main origin/fix/scheduler-stale-kick-test-context hits the same conflict, so this is fix(test): mark the stale-refresh test context as an unrestricted reader #965 vs the already-merged test(scheduler): kick the stale refresh with an unrestricted reader #960, not this PR.

Labels

I can't apply labels. Could a maintainer mirror #962's labels onto this PR: impact/internal, triaged, priority/p3, severity/low, urgency/this-quarter, effort/xs, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 4f9b107c-8546-42d5-b0d7-6f8d8558b6be

📥 Commits

Reviewing files that changed from the base of the PR and between 2ef2f5d and 83459e4.

📒 Files selected for processing: 2

• internal/scheduler/background_collection_test.go
• internal/scheduler/scheduler_test.go

+-----------------------------+
| ░R░e░v░i░e░w░ ░i░n░ ░b░i░o░ |
+-----------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

✨ 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.}

[cristim]

Independent review verdict: CLEAN at 83459e4

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

Blocking findings: none. Each test now fails when its isLambda guard is removed, and the same mutants survive on origin/main's tests, which confirms they were vacuous before.

Non-blocking notes:

• background_collection_test.go:161: deleting <-release as a harness-weakening probe still passed -count=100, so the positive control would not reliably catch a harness regression. The shipped block does make the flag check deterministic.
• The Lambda subtest's time.After(200ms) adds about 0.2s on the pass path. It is not a race, because the collecting assertion is synchronous.
• The two ListRecommendations tests overlap on the outer guard (scheduler.go:1183). Guard 1452 is covered only by the cold-start test, and guard 1502 only by the new direct test.

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

• AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test -race -count=1 ./internal/scheduler/: 199 passed, no SKIP.
• go test -race -count=50 -run 'Lambda|LambdaGuard' ./internal/scheduler/: 300 passed, with no hang or race.
• Mutants: each guard was removed in turn in a scratch worktree and restored after.
  • Outer guard (scheduler.go:1183): LambdaSkipsBackgroundRefresh fails ("GetGlobalConfig was called 1 time(s)... expected none"), and ColdStart_Lambda fails too.
  • kickColdStartCollection guard (:1452): ColdStart_Lambda_NeverMarksOrCollects fails cleanly on MarkCollectionStarted, with no panic.
  • maybeKickBackgroundRefresh guard (:1502): TestMaybeKickBackgroundRefresh_LambdaGuard/lambda_never_kicks fails, and the non-Lambda control still passes.
  • The same three mutants all survive on origin/main's test files.
• Cleanup: t.Cleanup(close(release)) unblocks the store call. started is buffered (1), the mock is .Maybe(), and the refresh ends on the post-release error. There was no hang or leak.

Gate:

• Labels are now mirrored from #962.
• CodeRabbit is green.
• CI is still running at this SHA.
• The PR is a draft: please mark it ready when done. It merges once CI is green at this head.