Repository navigation
fix(purchase): skip scheduled purchases due more than 24h ago in the fire and process sweeps - #1549
Conversation
…fire and process sweeps The fire and process sweeps ship opt-in on most platforms, so the first tick after one is scheduled can find a long backlog of overdue rows. Until now every one of them was bought on that tick, on whatever recommendations and prices the row was created with. Both sweeps now check the row their "running" claim returned, right before the provider call. A row due more than the cap ago (scheduled_execution_at for a delayed purchase, scheduled_date otherwise) is refused before submission through the existing failClaimedBeforeSubmission path. It ends failed with no_submission set and the reason in its error, its suppressions are released, and operators get the purchase-failed alert once. It can be retried from History. The check runs after the claim, so overlapping sweeps, a revoke or an approve can't buy or double-report a row the cap refused. Rows still awaiting human approval are never claimed by the sweep, so the cap doesn't touch them. The SQS execute_purchase path keeps no cap. The cap defaults to 24h (matching LadderFireGrace) and can be set with SCHEDULED_PURCHASE_MAX_OVERDUE (a Go duration). A malformed or non-positive value fails startup. FireResult and ProcessResult report the refused rows as stale. Refs LeanerCloud#712 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. Finishing Touches
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 |
govulncheck in the Docker image build now fails on these two advisories, both fixed in x/net v0.62.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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: rG3. 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. |
|
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: rH1. 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: 2 approving independent review(s) on |
…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>
…in gateHarness Main's TestProcessScheduledPurchases_CapNeverTouchesRowsAwaitingApproval (LeanerCloud#1549) drives gateHarness with an AutoPurchase=false plan. The gate skips a manual plan before it reads the global config, so the required GetGlobalConfig expectation failed under assertMocksOnCleanup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…1570) * test(purchase): assert mock expectations across the purchase tests Every internal/purchase test that registers a required .On(...) now fails when that call never happens. A shared assertMocksOnCleanup(t, mocks...) helper registers AssertExpectations as a t.Cleanup at mock construction (or in the shared harness: newApproveManager, armedHarness, newDetachFixture, attachManager callers, tokenCapture, runFanOutCapturingOpts and others). Expectations that turned out never to fire were stubs for a path the test does not take, not missed production calls. They are now .Maybe() with a comment, or made conditional on the case that takes the path: - newPinManager: per-path entry reads (cron lists vs SQS GetExecutionByID) - gate tests: the kill-switch read for a missing plan (refused first) - traps the test asserts NotCalled (plan lookup for "", the running CAS, the failed-email for non-failed statuses, non-alerting writers) - shared stubs on paths that never reach them (optionalCalls helper) No production code changes. Refs #35 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(purchase): make the kill-switch read optional for a manual plan in gateHarness Main's TestProcessScheduledPurchases_CapNeverTouchesRowsAwaitingApproval (#1549) drives gateHarness with an AutoPurchase=false plan. The gate skips a manual plan before it reads the global config, so the required GetGlobalConfig expectation failed under assertMocksOnCleanup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…ons at 02:00 UTC (#1580) * fix(iac): fire delayed purchases by default; collect AWS recommendations at 02:00 UTC Owner decision D33 (#712): the fire_scheduled_purchases sweep is now on by default on every platform. enable_fire_scheduled_purchases_schedule defaults to true in the aws, azure and gcp environment roots and in the Lambda, Fargate, Cloud Run, GKE, Container Apps and AKS modules, and the CloudFormation EnableFireScheduledPurchasesSchedule parameter defaults to "true". The sweep only executes purchases already approved for a later time, and the 24h due-row age cap (#1549) refuses and alerts on anything older, so the first run cannot buy a stale backlog. The process sweep and every other opt-in toggle stay off. The Go policy test now treats fire as default-on (it would flag a committed AWS tfvars that turns it off), and the module terraform tests expect the fire job by default and cover the opt-out. Owner decision D32 (#756): the AWS recommendation_schedule default moves from rate(1 day) to cron(0 2 * * ? *), 02:00 UTC like GCP and Azure, in the env root, the Lambda and Fargate modules, the github-*.tfvars and the example tfvars. A rate() schedule has no known phase, so the freshness endpoint's next_collection_at was always null on AWS. A new test reads every default and committed value and checks the API's NextCollectionAt returns the next 02:00 UTC for each. Refs #712 Refs #756 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(changelog): note that bastion target roles must trust the bastion #1556 assumes a bastion account's target role with the bastion's own credentials and no longer falls back to the CUDly host's, so a target role that trusts the host instead of the bastion now gets AccessDenied until its trust policy names the bastion. Refs #712 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * revert(cfn): leave the CloudFormation install stack to its retirement #1432 (D29) deletes cloudformation/stacks/CUDly/ and scheduled_task_cfn_test.go, so the CloudFormation edits in this branch would only cause modify/delete conflicts. Restore both files to main and drop CloudFormation from the CHANGELOG entry. The fire sweep's default changes on the Terraform platforms only. Until #1432 lands, TestCloudFormationSchedulesEveryRequiredTask and the "opt-in task on by default" negative in that file fail on this branch alone, because the policy now treats fire as default-on and the retiring template still defaults it off; #1432 removes both tests. Refs #712 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(iac): give the schedule-default test its own default regex TestRecommendationScheduleDefaultsReportNextCollection borrowed tfStringDefaultRe from scheduled_task_cfn_test.go, which #1432 deletes; the combined tree would not compile. Declare a local copy instead. Refs #712 Refs #756 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Root cause
fire_scheduled_purchasesandprocess_scheduled_purchasesship opt-in on most platforms (maintainer decision on #712). When a deployment first schedules one, the first tick finds every row that became due while it was off. Both sweeps bought all of them right away, on whatever recommendations and prices each row was created with. Nothing limited how late a row could be. The maintainer's scope comment on #712 says flipping fire/process on by default waits on this due-row age cap.Change: item 4 of the issue only
runningclaim returned, immediately before the provider call. The fire sweep does this infireOneDue. The process sweep uses a newclaimAndExecuteWithin(claimAndExecutewith a cap), called fromprocessOneExecution.status = scheduled) the row is due atscheduled_execution_at. For every other row it is due atscheduled_date.failClaimedBeforeSubmissionpath, the same one the post-claim step-cap and authorization refusals use. No new status is added. The row ends:failedwithno_submission = true;error, which History shows. Example:scheduled purchase skipped: it was due too long ago: it was due at 2026-10-25T10:00:00Z, 49h0m0s ago, which is more than the 24h0m0s limit (SCHEDULED_PURCHASE_MAX_OVERDUE); nothing was submitted, retry it if the purchase is still wanted;purchase_execution_failedmarker and the purchase-failed email (chore(iac): no alert reaches anyone; instantiate monitoring and wire failure notifications #714/fix(purchase): alert on failed fan-out runs whose children never notify #1195);failedand notexpired.expiredmeans "approval lapsed before anyone acted". It is written only for pending/notified rows, carries no reason and sends no alert. It also cannot be retried. Every existing "refused before submission" path endsfailed+no_submission. Ladder reconcile, ramp-blocked health and the retry flow already treat that state as "nothing bought".config.LadderFireGrace, the ladder's limit for firing its own overdue tranches. Override it withSCHEDULED_PURCHASE_MAX_OVERDUE, a Go duration such as36h. A malformed or non-positive value fails startup, likeDEFAULT_TERM. When the field is unset, or the Manager is built directly, the default cap applies; there is no "no cap" setting.FireResult.StaleandProcessResult.Stalereport refused rows, and the handler log lines print them.Failed, as the other post-claim refusals do. So a process tick that refuses a backlog returnsErrScheduledPurchasesFailedonce (chore(iac): no alert reaches anyone; instantiate monitoring and wire failure notifications #714: partial failure must not read as success). Later ticks are clean because the refused rows are terminal.Concurrency (money path)
The check runs after the claim CAS (
scheduled -> running, orapproved/pending/notified -> running), so whichever sweep wins the claim alone decides the row:scheduled -> canceled) wins, the sweep loses the claim. If the sweep wins, the revoke gets its existing 410 and the row then ends failed, unbought.time.Now()after the claim. A row cannot age past the cap between a stale listing and the purchase, and a listed row read inside the cap is re-checked after its claim.runningfor the reaper (existing behavior for every post-claim refusal).The integration test runs 4 process sweeps and 4 fire sweeps at once over a backlog of overdue and fresh rows on real Postgres.
What the cap does not touch
pendingrow, or a row while the kill switch is off. The scheduler gate skips these before any claim, so an overdue notified row on a manual plan stays approvable by email or session until its own token TTL.execute_purchase. It sharesclaimAndExecutebut passes no cap. It is not one of the two sweeps, and it executes an explicitly enqueued row.RecoverStrandedApprovalshandles human-approved rows that were interrupted mid-run, with its own threshold.Verification
All evidence below is unit tests with mock store and provider, plus integration tests on real Postgres (testcontainers) with the ramp-step fixture's counting fake provider. No cloud API was called and nothing was purchased.
Environment:
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.9.Fail-before / pass-after (Postgres integration). The new
due_age_cap_integration_test.gouses only APIs that exist on main. It was copied unchanged into a clean worktree of main at7b86e386and run there:TestDueRowAgeCap_OverlappingSweepsBuyOnlyRowsInsideTheCapFiredexpected 1, got 2. Both overdue rows (notified 48h late, scheduled 48h late) endedcompletedwith no reason and no failure email.failedwithno_submissionand the reason, and each got exactly one failure email across 8 overlapping sweeps.TestDueRowAgeCap_RefusedRowsStayRefusedUnit tests (
due_age_cap_test.go; they use the new symbols, so they don't compile on main). They cover:scheduled_execution_atoverridingscheduled_date;no_submission, reason, suppressions released, one email, provider never reached, noapproved_bystamp;internal/server/app_test.gocovers the env var: default,36h, and rejection of24,1d,0sand-1h.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@400d256b= origin/main7b86e386plus 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 onf7c48020, 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: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/analytics,internal/api,internal/auth,internal/config,internal/database/postgres/migrations,internal/purchase,internal/schedulerandinternal/serverall pass. Migrations 000149, 000150 and 000151 apply in order on this tree.approval-details,settings-accounts,api) pass (221 tests).accountkey); fixed with named constants. Re-run on this tree: git-secrets, using the repo'sscripts/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 locally (CI runs them). Nothing was applied to any cloud.Out of scope (deliberately left out)
SCHEDULED_PURCHASE_MAX_OVERDUE; the 24h default applies everywhere until a deployment sets it.NewApplicationpassesScheduledPurchaseMaxOverdueintopurchase.ManagerConfig(the field is unexported on the Manager). The two wiring lines are ininternal/server/app.go.execute_purchasepath has no due-date or age check at all. That is a pre-existing gap: it is only reached by explicitly enqueued rows.Frontend follow-up for the UI owner: none required. History already shows a failed row's
errortext and offers Retry. An optional later nicety would be a distinct badge for "skipped: overdue" failures.Refs #712
Maintainers: please mirror #712's triage labels onto this PR:
priority/p1,severity/high,urgency/this-sprint,impact/many,effort/m,type/bug,triaged.🤖 Generated with Claude Code