Repository navigation
feat(api): report the next scheduled collection in recommendations freshness - #1567
Conversation
…eshness GET /api/recommendations/freshness now carries next_collection_at so the first-run empty state can say when recommendations will next be collected. The runtime did not know its own schedule, so every compute module that schedules collect_recommendations (Lambda, Fargate, Cloud Run, GKE, AKS, Container Apps) now passes it as RECOMMENDATION_SCHEDULE, empty when the scheduler resource is not created. A new internal/schedexpr package parses EventBridge rate()/cron() and five-field cron and computes the next UTC fire time. rate() schedules report null: EventBridge counts them from rule creation, which the server cannot see. Unset, disabled or unparsable schedules also report null. Container Apps passes the daily recurrence its Logic App really runs (hour field only), not the raw cron. Refs LeanerCloud#756 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…llection Conflict in internal/server/app_test.go (TestLoadApplicationConfig): kept both RECOMMENDATION_SCHEDULE (this branch) and main's SCHEDULED_PURCHASE_MAX_OVERDUE (LeanerCloud#1549) in the env list, setters and assertions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou'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. View limit details
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. Comment |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rI1. 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. |
|
Local verification at |
|
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 |
#1389 added the first-run empty state, which shows the last collection time. The issue also asks for the next one, but the API had no field for it, and the server did not know its own schedule. Terraform gave the schedule to EventBridge, Cloud Scheduler, CronJobs or the Logic App, never to the app. This PR adds
next_collection_attoGET /api/recommendations/freshnessand wires the schedule through to the server.Refs #756
Partial: this is the backend half. Showing the time on the empty-state card is a frontend change for the UI owner (see below).
Root cause
config.RecommendationsFreshnessonly hadlast_collected_at,last_collection_error,last_collection_started_atandlast_collection_partial. The collection schedule lived only in Terraform (var.recommendation_schedule), so nothing at runtime could work out when the next run was due.What changed
collect_recommendationsnow setsRECOMMENDATION_SCHEDULEon the API container. The value isvar.enable_scheduled_tasks ? var.recommendation_schedule : "", so it is empty whenever the scheduler resource is not created:modules/compute/aws/lambda/main.tfandaws/fargate/main.tf(EventBridgerate()/cron(), UTC);gcp/cloud-run/main.tfandgcp/gke/main.tf(Cloud Scheduler 5-field cron,time_zone = "UTC");azure/aks/main.tf(local.app_env, shared with the CronJobs, 5-field cron, UTC).azure/container-apps/main.tfis different. Its Logic App reads only the hour field and runs daily at<hour>:00 UTC(scheduled-tasks.tf), so the env var is"0 ${local.schedule_hour} * * *", the schedule that actually runs, not the raw cron.internal/schedexprpackage (schedexpr.goandcron.go).Parseacceptsrate(N unit), EventBridgecron(min hour dom month dow year)and 5-field cron. It supports*,?, lists, ranges, steps and month/day names, and maps EventBridge day-of-week 1-7 (Sunday is 1) and classic 0-7 (Sunday is 0 or 7).Next(t)returns the first fire time strictly aftert, in UTC, and searches up to 10 years ahead. The parser is strict. Anything it does not fully support is an error:L,W,#,@daily, backwards ranges, signed numbers, a wrong field count, or an EventBridge cron where both or neither day field is?.rate()always gives null. EventBridge counts a rate schedule from the moment the rule was created, and the server cannot see that. Usinglast_collected_atas the starting point would also be wrong, because manual refreshes and the 6h on-read background refresh (non-Lambda) move it. So the field reports "unknown" instead of guessing.HandlerConfig.RecommendationScheduleis read fromRECOMMENDATION_SCHEDULEinLoadApplicationConfig. It is passed to bothNewHandlercall sites (pre-connect andreinitializeAfterConnect) and parsed once per handler. An unparsable value logs one warning and is treated as unset.getRecommendationsFreshnesssetsNextCollectionAtfrom the handler clock. The value is computed, not stored, so there is no migration and no DB write. The store returns a new struct on every call, so setting the field does not race with other requests.internal/api/openapi.yaml: documentsnext_collection_at(nullabledate-time, always present, so it is listed inrequired) and when it is null..env.examplehas a commented example.Verification
Unit, mock and fixture evidence only. Nothing was deployed and no Terraform plan or apply was run.
Commands (with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.9):go build ./...,go vet ./...,go vet -tags integration ./...: clean.go test -count=1run one package at a time:./internal/schedexpr/,./internal/api/,./internal/server/,./internal/config/,./terraform/modules/compute/,./internal/mocks/. Allok.golangci-lint run --allow-parallel-runners(v2.10.1) oninternal/schedexpr,internal/api,internal/server,internal/configandterraform/modules/compute: 0 issues.gocyclo -over 10 -ignore "_test\.go"on the changed non-test files: nothing over 10.terraform fmt -check -recursive terraform/modules/compute: clean.terraform init -backend=falseplusterraform validateon temp copies ofaws/lambda,aws/fargate,gcp/cloud-run,gcp/gke,azure/aksandazure/container-apps: "Success! The configuration is valid." for all six.New tests:
internal/schedexpr/schedexpr_test.go:ratewith minute(s), hour(s) and day(s): the period is parsed andNextreports unknown.rate: zero, negative,+2, singular/plural mismatch, weeks, non-integer, malformed.?and*,0/15, lists, names, numeric day-of-week (2 is Monday), the year field, 29 February (gives 2028), "strictly after", and a non-UTC input evaluated as UTC.*/30, day-of-week 0 and 7 as Sunday, and OR matching when both day fields are restricted.internal/api/recommendations_next_collection_test.go: the freshness JSON gives"2026-10-28T02:00:00Z"forcron(0 2 * * ? *)and"2026-10-27T18:00:00Z"for0 */6 * * *at a fixed clock. It givesnull, with the key present, for unset,rate(1 day), unparsable and no-run-in-horizon schedules.NewHandlerparsesHandlerConfig.RecommendationSchedule.internal/server/app_test.go:LoadApplicationConfigreadsRECOMMENDATION_SCHEDULE, andNewApplicationFromDepspasses it to the API handler.terraform/modules/compute/recommendation_schedule_env_test.go: each of the six modules has exactly oneRECOMMENDATION_SCHEDULEentry with the expected gated expression. A shape test covers the map, ECS and AKS-list forms of the matcher.Fail-before / pass-after:
7b86e386) with a probe test callinggetRecommendationsFreshness: body{"last_collected_at":null,...,"last_collection_partial":null}givesFAIL: response must carry "next_collection_at". On this branch the same probe passes with"next_collection_at":null.undefined: parseCollectionSchedule,ApplicationConfig has no field ... RecommendationSchedule).TestRecommendationScheduleEnvWiringfails for all six modules with "want exactly one RECOMMENDATION_SCHEDULE env entry, got 0".TestFreshnessNextCollectionAtfails;NewApplicationFromDepswiring:TestNewApplicationFromDepsfails;TestLoadApplicationConfigfails;TestNextEventBridgeCronfails;TestNextUnixCronfails;Nextinclusive oft:TestNextEventBridgeCronfails;?" rule:TestParseCronInvalidfails;ratereturn a time:TestParseRatefails;TestRecommendationScheduleEnvWiringfails.Not covered by a test: the
reinitializeAfterConnectcall site (it needs a DB). It reads the sameappConfigfield the tested pre-connect path uses.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@c00ef619= origin/main15dc8febplus these heads, merged in this order: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 on644bfcaf, which differs from this tree only by a test-only rename on fix/issue-134-mfa-token (a password-like constant that git-secrets flagged):go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....--build-tags integration.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1):internal/api,internal/auth,internal/config,internal/database/postgres/migrations,internal/purchaseandinternal/serverall pass. Migration 000152 applies on top of main's 000151.scripts/setup-git-secrets.shpatterns 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..tftest.hclfiles were not run on this tree (CI runs them). Nothing was applied to any cloud.Left out of scope
rate(1 day), so a default AWS deployment reportsnext_collection_at: null. Fixing that means switching the AWS default to a cron such ascron(0 2 * * ? *)(02:00 UTC, the same as GCP and Azure). That moves the collection time of existing deployments, so it is an owner decision and is not changed here.cloudformation/stacks/CUDly/template.yaml) also schedules recommendation collection and does not setRECOMMENDATION_SCHEDULEyet. The brief covered Terraform only. A CFN follow-up would add the env var from the same schedule parameter.next_collection_atis the next scheduled run only. Manual refreshes and the non-Lambda on-read background refresh (CUDLY_RECOMMENDATION_CACHE_TTL, default 6h) are not predicted.nulluntil Terraform is applied again and the new env var appears.Frontend follow-up for the UI owner
frontend/src/api/recommendations.ts: addnext_collection_at?: string | nulltoRecommendationsFreshness.frontend/src/first-run-empty-state.ts: next to the last-collection line (around line 52,formatDateTime(f.last_collected_at)), show "Next collection: <formatDateTime(next_collection_at)>" when the field is non-null. Show nothing when it is null, which is what everyrate()schedule and disabled scheduling return. Hide it while a collection is running (last_collection_started_atset).Maintainer: please mirror the issue's triage labels onto this PR:
priority/p2,severity/medium,urgency/this-quarter,impact/many,effort/m,type/bug,triaged.🤖 Generated with Claude Code