Skip to content

fix(iac): fire delayed purchases by default; collect AWS recommendations at 02:00 UTC - #1580

Merged
IAmJSD merged 6 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-712-fire-default-aws-schedule
Oct 28, 2026
Merged

IAmJSD merged 6 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-712-fire-default-aws-schedule

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Implements two acting-owner decisions (IAmJSD, 2026-10-28) on top of the sweep scheduling that #1549, #1550 and #1573 already landed for #712, and adds one release notice the coordinator asked for.

Refs #712
Refs #756

After this merges, #712 can be closed. Its last open item, turning the fire sweep on by default, is done here, and every platform already schedules every task. The process sweep staying opt-in is the owner's decision, not leftover work. One gap stays documented in knownGaps: the GKE module deployed from the GCP environment root gets no enable_scheduled_tasks/app_url, so it schedules nothing until someone picks the URL Cloud Scheduler should call. That needs its own decision and is not part of this issue's remaining scope.

D33: fire_scheduled_purchases sweep on by default (#712)

Root cause. enable_fire_scheduled_purchases_schedule defaulted to false everywhere. A delayed auto-approved purchase (status scheduled, with scheduled_execution_at set) never ran unless an operator had turned the sweep on, and the GCP/Azure github environments had not.

Fix.

  • enable_fire_scheduled_purchases_schedule now defaults to true in terraform/environments/{aws,azure,gcp}/variables.tf and in each module that declares it: AWS Lambda, AWS Fargate, GCP Cloud Run, GCP GKE, Azure Container Apps and Azure AKS. The descriptions now say why turning it on is safe. Since fix(purchase): skip scheduled purchases due more than 24h ago in the fire and process sweeps #1549 the server refuses any row due more than 24h ago (SCHEDULED_PURCHASE_MAX_OVERDUE): the row is marked failed with no_submission, operators get the purchase-failed alert, and a stale backlog is never bought.
  • The CloudFormation install stack is out of scope: fix(cfn): CloudFormation CUDly stack can't start the current server (no DB_HOST; still provisions DynamoDB) #1432 (D29) retires it.
  • Every process-sweep toggle stays false. No other opt-in default changed (send_notifications, cleanup, ladder_run, ri_exchange).
  • Comments that called fire opt-in are updated: the Lambda main.tf rule header, the sweep_tasks locals in Cloud Run, GKE, Container Apps and AKS, and the ErrDueRowTooOld doc comment.
  • The policy tests in terraform/modules/compute take fire_scheduled_purchases out of optInRequired. The env/module default checks and the committed-AWS-tfvars check now treat it as default-on. The negative fixtures that used fire as their opt-in example now use process. A new negative covers "a tfvars file switches fire off", plus an explicit assertion that fire is default-on and process is opt-in.
  • Module terraform test files (Lambda, Fargate, Cloud Run, Container Apps, AKS) now expect the fire job in the default plan. Each one also gets an opt-out run (enable_fire_scheduled_purchases_schedule = false removes the rule, target and permission, the job and its OIDC audience, or the workflow), or an existing all-off run now covers fire.

What each environment does on its next apply. No committed tfvars (the github-*.tfvars, dev.tfvars.example or terraform/profiles/*) sets enable_fire_scheduled_purchases_schedule or enable_process_scheduled_purchases_schedule, so the new default applies everywhere:

  • AWS dev/staging/prod (compute_platform = "lambda"): creates the <stack>-fire-scheduled-purchases EventBridge rule (rate(15 minutes)), its target and its Lambda permission. A Fargate deploy creates the rule, the ECS target and the invoker role/policy instead.
  • GCP dev/staging/prod (cloud-run, enable_scheduled_tasks = true): creates the <service>-fire Cloud Scheduler job (*/15 * * * *) and adds its audience to SCHEDULED_TASK_OIDC_AUDIENCE, which rolls a new Cloud Run revision.
  • Azure dev/staging/prod (container-apps, enable_scheduled_tasks = true): creates the fire Logic App workflow (every 15 minutes) with its user-assigned identity, Key Vault Secrets User assignment and actions.
  • Process sweep: unchanged everywhere (off).

D32: AWS collection at 02:00 UTC (#756)

Root cause. The AWS recommendation_schedule default was rate(1 day). A rate schedule's phase depends on when the rule was created, which the API cannot see, so GET /api/recommendations/freshness always returned next_collection_at: null on AWS (PR #1567).

Fix. The default is now cron(0 2 * * ? *) (02:00 UTC, the same as GCP and Azure) in:

  • terraform/environments/aws/variables.tf
  • the Lambda and Fargate module variables
  • terraform/environments/aws/github-{dev,staging,prod}.tfvars
  • the AWS example tfvars (environments/aws/dev.tfvars.example had rate(24 hours), and profiles/aws/*.tfvars.example), so copies of them don't bring back the null

The backend needed no change: schedexpr already parses EventBridge cron, and internal/api's TestFreshnessNextCollectionAt already covers cron(0 2 * * ? *) → 2026-10-28T02:00:00Z. The new test TestRecommendationScheduleDefaultsReportNextCollection (terraform/modules/compute/recommendation_schedule_env_test.go) ties the IaC to the API. It reads the default in the AWS, GCP and Azure env roots and the Lambda and Fargate modules, plus every committed AWS tfvars value. It runs each one through api.NewHandler(...).NextCollectionAt and requires the next 02:00 UTC. It also fails if a github-*.tfvars stops setting the value.

On the next AWS apply, the collection rule's schedule_expression changes in place. The RECOMMENDATION_SCHEDULE env var updates the Lambda function in place; on Fargate it registers a new task definition and rolls the service.

D27 notice: bastion target roles (#1556)

The coordinator asked for a CHANGELOG notice after #1556 (Refs #263) merged: CUDly now assumes a bastion account's target role with the bastion's own credentials and no longer falls back to the CUDly host's. A bastion account whose target role trusts the CUDly host instead of the bastion now gets AccessDenied until the bastion is added as the trusted principal in that role's trust policy. The notice is under ## [Unreleased] → ### Notices, linking #1556. This branch merged origin/main (6e2033c) to pick it up.

CHANGELOG

  • Unreleased → Changed: the fire sweep is now on by default (with how to opt out), and AWS collection moves to 02:00 UTC.
  • Unreleased → Notices: the bastion trust-policy change above.

Verification

All of this ran locally on Linux. It is unit, fixture and mocked-provider evidence; nothing touched a real cloud account and nothing was planned or applied against one.

Fail-before: I reverse-applied this branch's IaC changes and kept the new tests, with go test -count=1 ./terraform/modules/compute/:

--- FAIL: TestRecommendationScheduleDefaultsReportNextCollection
    ../../environments/aws/variables.tf: recommendation_schedule "rate(1 day)" gives next_collection_at <nil>; want 2026-10-28 02:00:00 +0000 UTC (daily at 02:00 UTC)
    aws/lambda/variables.tf, aws/fargate/variables.tf, github-{dev,staging,prod}.tfvars, the example tfvars: same
--- FAIL: TestEnvWiring
    aws-lambda / aws-fargate / gcp-cloud-run / gcp-gke / azure-container-apps / azure-aks:
    fire_scheduled_purchases: enable var defaults to false, policy wants default-on=true

Same revert, mocked terraform test -filter=<file>: the defaults run fails in Lambda sweeps, Fargate sweeps, Cloud Run scheduled_tasks, Container Apps scheduled_tasks and AKS scheduled_tasks.

Pass-after, at the branch head:

  • go build ./..., go vet ./..., go vet -tags integration ./...: clean
  • go test -count=1 ./terraform/modules/compute/: everything passes except two tests in scheduled_task_cfn_test.go, which fix(cfn): CloudFormation CUDly stack can't start the current server (no DB_HOST; still provisions DynamoDB) #1432 deletes. This branch must merge after fix(cfn): CloudFormation CUDly stack can't start the current server (no DB_HOST; still provisions DynamoDB) #1432. TestCloudFormationSchedulesEveryRequiredTask and the "opt-in task on by default" case of TestCloudFormationScheduleCheckNegatives fail on this branch alone, because the policy now treats fire as default-on while the retiring template, left at main's content, still defaults it off. On a local merge of this branch with fix/issue-1432-retire-cfn-stack (9cb7de8), the merge is clean (no conflicts), go build ./... passes, and go test -count=1 ./terraform/modules/compute/... is all ok.
  • go test -count=1 ./internal/purchase/: ok
  • go test -count=1 ./internal/api/ -run 'Freshness|NewHandlerParsesRecommendationSchedule': ok
  • go test ./internal/mocks/ -count=1: ok
  • golangci-lint run --allow-parallel-runners ./terraform/modules/compute/... ./internal/purchase/... (v2.10.1): 0 issues
  • gocyclo -over 10 -ignore "_test\.go" internal/purchase/due_age_cap.go: nothing over 10
  • terraform fmt -recursive -check terraform/: clean
  • terraform init -backend=false && terraform validate in terraform/environments/{aws,azure,gcp} and terraform/modules/compute/{aws/lambda,aws/fargate,gcp/cloud-run,gcp/gke,azure/container-apps,azure/aks}: all valid
  • terraform test (every provider mocked) in each touched module with tests: Lambda 16/16, Fargate 4/4, Cloud Run 10/10, Container Apps 14/14, AKS 14/14 passed. GKE has no tftest files.

Combined-tree verification. Tested on a local, never-pushed tree: queue/platform @ 40dbbdaf = origin/main 6e2033cd plus:

Branch Tested head
fix/issue-1494-listing-settlements d7aa121
fix/issue-1529-ambiguous-return 8ecf506
fix/issue-1434-lambda-budget 8b6eebe
fix/issue-1432-retire-cfn-stack 9cb7de8
fix/issue-712-fire-default-aws-schedule 8bd7095

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.

Out of scope (deliberately not changed)

  • Turning the process sweep on, and Lambda timeouts.
  • internal/config/defaults.go still stores scheduling.collect_schedule = "rate(1 day)" as a settings default. It does not drive any scheduler.
  • GKE from the GCP env root still schedules nothing (no app_url). This is documented in knownGaps and needs its own decision.

Frontend follow-up for the UI owner

No UI change is needed for this PR. The remaining #756 item (showing next_collection_at on the empty-state card) is still with the UI owner, and AWS now returns a real value for it.

Review notes

Maintainer: please mirror #712's triage labels onto this PR: priority/p1, severity/high, urgency/this-sprint, impact/many, effort/m, type/bug, triaged (#756 is p2/medium, so the higher #712 set covers it).

🤖 Generated with Claude Code

IAmJSD and others added 5 commits October 28, 2026 21:14
…ons at 02:00 UTC

Owner decision D33 (LeanerCloud#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 (LeanerCloud#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 (LeanerCloud#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 LeanerCloud#712
Refs LeanerCloud#756

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LeanerCloud#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 LeanerCloud#712

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LeanerCloud#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 LeanerCloud#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; LeanerCloud#1432 removes both tests.

Refs LeanerCloud#712

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TestRecommendationScheduleDefaultsReportNextCollection borrowed
tfStringDefaultRe from scheduled_task_cfn_test.go, which LeanerCloud#1432 deletes;
the combined tree would not compile. Declare a local copy instead.

Refs LeanerCloud#712
Refs LeanerCloud#756

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added urgency/this-sprint Within the current sprint triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm impact/many Affects most users effort/m Days type/bug Defect labels Oct 28, 2026
@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • 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

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

@IAmJSD

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

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

Blocking:

Non-blocking:

Attacked:

  • Fail-before: I reverted the non-test Terraform changes (variables, tfvars, examples, main.tf and sweep_tasks comments) and kept the new tests. TestRecommendationScheduleDefaultsReportNextCollection failed on every AWS default and tfvars file, and TestEnvWiring failed with "fire_scheduled_purchases: enable var defaults to false". Both pass at the head.
  • Checked that only the fire toggle changed: process, send_notifications, cleanup, ladder_run and ri_exchange still default off. Fire stays gated the same way as the other default-on sweeps (reap and analytics on Lambda/Fargate; enable_scheduled_tasks plus the per-task flag on Cloud Run, GKE, Container Apps and AKS). No committed tfvars sets the fire or process toggle. The 24h overdue cap (dueRowAgeRefusal on the claimed row, then failed with no_submission and an alert) runs before any provider call, so the first tick cannot buy a stale backlog.
  • Ran: go build ./..., go vet ./..., go vet -tags integration ./...; go test -race -short -count=1 ./terraform/modules/compute/... ./internal/purchase/ (only the two CloudFormation tests above fail); go test -race -tags integration -count=1 ./internal/purchase/ (ok); golangci-lint v2.10.1 on ./terraform/modules/compute/... (0 issues); mocked terraform test in aws/lambda (sweeps: 3 passed) and gcp/cloud-run (scheduled_tasks: 4 passed). Every provider was mocked, nothing touched a real backend, and init -backend=false was used.
  • Combined tree: this head merged with fix(cfn): retire the CloudFormation install stack #1577, fix(purchase): shorten execution budget to 3m30s so the failure alert fits the Lambda timeout #1578 and fix(revoke): keep the in-flight flag on an ambiguous Azure Return failure and refuse a late Lambda claim #1579. The merge is clean, and go test -race -short ./terraform/modules/compute/... ./internal/purchase/ is all ok.

Body-stale:

  • none

Reviewer: rM2. 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

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

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

Blocking:

Non-blocking:

  • internal/purchase/scheduled_fire.go:76-87 (pre-existing, but this PR turns it on by default on Lambda, Cloud Run and Container Apps): FireScheduledDelayedPurchases fires due rows one after another, each through executeAndFinalize with its own detached 270s budget, and never checks how much invocation time is left before it claims the next row. On Lambda (300s timeout), a tick that finds two due rows where the first purchase is slow can claim the second at, say, t=250s and get killed partway through its provider call. That row stays running, the reaper fails it later, and operators get the outcome-unknown email. The stable per-rec idempotency key makes a double purchase unlikely, but the audit row can disagree with the cloud. Suggested follow-up: stop claiming new rows when the remaining deadline is below the purchase budget (the same idea as fix(revoke): keep the in-flight flag on an ambiguous Azure Return failure and refuse a late Lambda claim #1579's checkRevokeTimeLeft), or fire one row per invocation on Lambda.
  • Deploy effect: every github-* environment gets new scheduler resources on its next apply. AWS gets an EventBridge rule, target and permission; GCP gets a Cloud Scheduler job, a changed SCHEDULED_TASK_OIDC_AUDIENCE and a new Cloud Run revision; Azure gets a Logic App, an identity and a Key Vault role assignment. On Fargate, the new RECOMMENDATION_SCHEDULE value also registers a new task-definition revision and rolls the service; the body calls that "change in place". The body lists all of this, but whoever applies it should know.
  • The old variable text said "Default false until auto-approve (L9) is promoted to GA". This PR flips the default without saying whether L9 is now GA. If auto-approve is still pre-GA, default-on means every install will fire any delayed rows it creates. That is the owner's call under D33, but it should be stated on purpose.

Attacked:

  • Money path: read the fire sweep. The scheduled→running CAS (claimRunning) prevents double firing across overlapping 15-minute ticks or a concurrent revoke. The 24h due-age cap is checked on the claimed row before any provider call and fails it no_submission with an alert, so turning the sweep on cannot buy a stale backlog. The process sweep and the other opt-in tasks stay off; the policy tests enforce that.
  • Fail-before: with this PR's tfvars and variables.tf changes reverted (git checkout origin/main -- on the env, profile and module variable files), TestRecommendationScheduleDefaultsReportNextCollection failed for all 11 AWS sources (rate(...) gives nil) and TestEnvWiring failed. Restored afterwards.
  • Combined: I merged fix(cfn): retire the CloudFormation install stack #1577 (9cb7de8) into a local scratch copy of this head. The merge was clean (CHANGELOG ends up with Notices, Changed, Removed and Fixed under Unreleased, no duplicate headings). go build ./... and go vet ./... were clean, golangci-lint v2.10.1 found 0 issues on terraform/modules/compute/... and internal/purchase, and go test -race -short -count=1 ./terraform/modules/compute/... ./internal/purchase/ ./internal/api/ was all ok. On the head alone: build, vet and vet -tags integration clean; only the two CFN tests above fail.
  • Mocked terraform test (every provider has a mock_provider; I checked before running): lambda sweeps 3/3, fargate sweeps 3/3, cloud-run scheduled_tasks 4/4, container-apps 10/10, aks 9/9. The new opt-out runs check that the rule, target and permission, or the job and OIDC audience, go away.
  • Grepped docs, README and known_issues for wording that still calls fire opt-in or gives rate(1 day) as the collection default: none left (the other rate(1 day) defaults belong to other sweeps).

Body-stale:

  • "On the next AWS apply ... the Lambda/ECS RECOMMENDATION_SCHEDULE env var change in place": true for Lambda. On Fargate it is a new task-definition revision and a service rollout.

Reviewer: rM1. 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

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

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

Blocking:

  • none

Non-blocking:

  • CHANGELOG.md:57 and the enable_fire_scheduled_purchases_schedule description in all nine variables.tf files (e.g. terraform/environments/aws/variables.tf:398, terraform/modules/compute/aws/lambda/variables.tf:249): "executes delayed auto-approved purchases" is wrong, and it matters for the opt-out decision. status = 'scheduled' is set only by approveWithDelay → scheduleApprovedExecution (internal/api/handler_purchases.go:1402/1452), which is reached only from the session approve path (:1197) and the email-token approve path (:1066), both after a person approves. GetScheduledExecutionsDue (internal/config/store_postgres.go:2253) reads only those rows. The delay is the global purchase_delay_hours (default 48 for a fresh install, internal/config/types.go:154), so on most deployments this sweep is what executes every human approval, not an auto-approve feature. An operator who reads "auto-approved", doesn't use auto-approve and sets the flag to false will leave every approval stuck in scheduled with no error. Suggested wording: "executes purchases a person approved with the purchase delay (purchase_delay_hours) once the delay has passed; with it off, delayed approvals never execute." The coverage-test comment (scheduled_task_coverage_test.go:28-31) already says it correctly. The pre-existing "(auto-approve delayed execution)" rule descriptions at aws/lambda/main.tf:788, aws/fargate/main.tf:1211 and the :778 comment block this PR edited have the same mix-up.
  • Now that this is on by default, fix(purchase): the fire sweep starts new purchases without checking the Lambda time left #1582 is reachable on AWS Lambda by default. FireScheduledDelayedPurchases claims due rows one after another without checking the Lambda time left, so a later row can be cut off mid-purchase and left running for the reaper. It is already filed and the body puts Lambda timeouts out of scope, so this doesn't block. Fixing fix(purchase): the fire sweep starts new purchases without checking the Lambda time left #1582 before the next prod apply would close the window.
  • Opting out (= false), enable_scheduled_tasks = false on GCP/Azure, and GKE from the GCP env root (knownGaps) all still let a user approve with a delay that never fires. fix(iac): several scheduled tasks are not scheduled; delayed purchases never fire on GCP/Azure #712's "reject purchase_delay_hours > 0 where there's no fire sweep" suggestion isn't done. It's pre-existing and the body says so; it's worth a follow-up issue so the opt-out isn't silent.
  • On the first apply, every scheduled row due more than 24h ago is failed and alerts operators, one email per row (up to MaxListLimit per tick). This is expected. Worth a sentence in the CHANGELOG entry so operators aren't surprised by a burst of alerts on deploy day.
  • Fargate: the default-on fire rule launches a one-off ECS task every 15 minutes (96 a day) even when nothing is due. The cost is small, and it follows D31/D33, but it's a new standing cost the CHANGELOG doesn't mention.
  • On an earlier review (rM1): it asked whether flipping the default needs auto-approve (L9) to reach GA. It doesn't. As the first item shows, the fire sweep doesn't depend on auto-approve; it executes human approvals held by purchase_delay_hours. The old "until L9 is GA" wording was itself the mix-up.

Attacked: I confirmed the head is d99c879; origin/main is one unrelated commit ahead (#1581) and merge-tree is clean. #1577 (CFN retirement) is in the head, so the merge-order caveat no longer applies. Traced every writer of status='scheduled' (only approveWithDelay), the fire claim, the 24h age cap and preExecutionRefusal in executeAndFinalize. Checked env→module wiring for fire on all six platforms. No committed tfvars/profile sets the fire or process flags, and nothing else overrides recommendation_schedule on AWS. #784 and #894, the prerequisites cristim named for HTTP money sweeps, are closed. Fail-before: I checked out the merge-base versions of every non-test terraform file the PR touches. TestRecommendationScheduleDefaultsReportNextCollection then fails for all 10 AWS sources and TestEnvWiring fails for all 6 platforms. Reverting only the three env-root fire defaults also fails TestEnvWiring, so the env roots are covered. Restored afterwards. Ran: go build ./...; go vet with and without -tags integration on ./terraform/... and ./internal/purchase; go test -race -short -count=1 ./terraform/... ./internal/purchase/ (all ok); go test -race -tags integration -count=1 ./internal/purchase/ (ok); golangci-lint v2.10.1 on both packages, with and without the integration tag (0 issues); mocked terraform test in lambda 16/16, fargate 4/4, cloud-run 10/10, container-apps 14/14, aks 14/14.

Body-stale:

Reviewer: rO2. 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

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

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

Blocking:

  • none

Non-blocking:

  • CHANGELOG.md:56 and the eight enable_fire_scheduled_purchases_schedule descriptions (e.g. terraform/environments/aws/variables.tf:398): "executes delayed auto-approved purchases" is misleading. The only writer of status=scheduled is approveWithDelay (internal/api/handler_purchases.go:1402, via the session path :1197 and the email-token path :1066), so this sweep fires purchases that a person approved while purchase_delay_hours > 0. That is how the D33 decision on fix(iac): several scheduled tasks are not scheduled; delayed purchases never fire on GCP/Azure #712 puts it, too. An operator who reads "auto-approved" may decide the sweep is irrelevant unless they use ladder auto_approve mode, and opt out. That opt-out would strand every delayed human approval, which then gets refused after 24h. Suggest "delayed purchases a person approved (purchase_delay_hours > 0)".
  • CHANGELOG.md:54-56: "on by default on every Terraform platform (… GKE …)". GKE deployed from the GCP environment root still gets no enable_scheduled_tasks/app_url and schedules nothing (the knownGaps entry the PR body mentions). A short caveat in the entry would avoid overclaiming.
  • Pre-existing, not introduced here, but it now matters on every deployment: fireOneDue (internal/purchase/scheduled_fire.go, "Stamp ApprovedBy") overwrites approved_by from the approving person's email to "scheduler" before the purchase. The row's own approver attribution is lost, and only the transition audit's transitioned_by for the scheduled transition is left (NULL on the email-token path). Worth a follow-up issue now that this path is on by default.

Attacked:

  • Money path: confirmed status='scheduled' is set only by scheduleApprovedExecution ← approveWithDelay (CAS from pending/notified, token pinned on the email path, cap/constraint checks before it). No auto-approve, ladder, ramp or migration path writes it. Fire claims each row with a scheduled→running CAS (no double fire across overlapping 15-minute ticks or Lambda/Fargate concurrency) and applies the 24h dueRowAgeRefusal on the claimed row, which fails it with no_submission and alerts once. Defaulting it on cannot buy a stale backlog, and process stays opt-in (optInRequired still lists process/send_notifications/cleanup).
  • Coverage: every module that declares the var (Lambda, Fargate, Cloud Run, GKE, Container Apps, AKS) and all three env roots flip to true, and the env roots pass the var through (compute.tf). No committed tfvars or profile sets it. The CFN stack is gone on main (fix(cfn): retire the CloudFormation install stack #1577), and scheduled_task_cfn_test.go no longer exists. The 02:00 cron change covers the env root, both AWS modules, the github-*.tfvars and every AWS example. Merging with current main (c2aaa0d) is clean.
  • Fail-before: in the scratch checkout I reverse-applied the 21 non-test IaC files, keeping the tests. go test ./terraform/modules/compute/ then failed TestRecommendationScheduleDefaultsReportNextCollection (all 11 sources give nil next_collection_at) and TestEnvWiring (6 modules: "fire … defaults to false, policy wants default-on"). After restoring, both pass.
  • Ran: go build ./..., go vet ./..., go vet -tags integration ./... (clean). go test -race -short -count=1 ./terraform/modules/compute/... ./internal/purchase/ (ok). go test -race -tags integration -count=1 ./internal/purchase/ (ok). golangci-lint v2.10.1 with and without --build-tags integration on those packages (0 issues). terraform fmt -recursive -check terraform/ (clean). Mocked terraform test passed in Lambda (16/16), Fargate (4/4), Cloud Run (10/10), Container Apps (14/14) and AKS (14/14).

Body-stale:

Reviewer: rO1. 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

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

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

Addendum to my rO1 review above (same head, same verdict). I skimmed the earlier reviews after writing mine and add two points that came from them:

Blocking:

  • none

Non-blocking:

  • (From earlier reviews.) FireScheduledDelayedPurchases claims due rows one after another and never checks how much invocation time is left. Now that fire is on by default, a Lambda tick (300s) with several slow due rows can be killed mid-purchase, which leaves a row running until the reaper fails it. This is tracked as fix(purchase): the fire sweep starts new purchases without checking the Lambda time left #1582. It doesn't block this PR, but fix(purchase): the fire sweep starts new purchases without checking the Lambda time left #1582 should be scheduled soon.
  • (From an earlier review; it makes my wording point stronger.) DefaultPurchaseDelayHours = 48 (internal/config/types.go:154). On a default install, every human approval goes through approveWithDelay and is executed only by this sweep. "Auto-approved" in the CHANGELOG and the variable descriptions therefore misdescribes the main purchase path, and an opt-out silently stops every approved purchase.

Attacked: the brief's claim that only approveWithDelay sets status=scheduled (see my review above). I also verified the 48h default in the code.

Body-stale: as in my review above.

Reviewer: rO1. 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

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at d99c879de9e9cb8aa1212c763d086eb1bdd1c957 merged onto main (c2aaa0d6), together with #1580 in that order (combined tree f5d4515f, 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
IAmJSD marked this pull request as ready for review October 28, 2026 22:53
@IAmJSD
IAmJSD requested a review from cristim as a code owner October 28, 2026 22:53
@IAmJSD
IAmJSD merged commit 2c43d8d into LeanerCloud:main Oct 28, 2026
41 checks passed
@IAmJSD

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

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 d99c879d (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly d99c879de9e9cb8aa1212c763d086eb1bdd1c957.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/many Affects most users priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant