Skip to content

fix(cfn): schedule the missing server tasks on the CloudFormation stack - #1300

Draft
IAmJSD wants to merge 3 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-712
Draft

IAmJSD wants to merge 3 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-712

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Part of #712. This is slice 4 of the "Scope for #712" plan: CloudFormation schedules, with cleanup as its own commit. Slices 1 to 3 are already on main (#902, #903, #939, #949, #986). The #784 change landed as #895, so nothing in the plan is blocked by it any more. This slice is AWS-only and was never blocked.

Root cause

cloudformation/stacks/CUDly/template.yaml only scheduled collect_recommendations (the ScheduledRule) and process_scheduled_purchases (the daily PurchaseCheckRule). Six other required server tasks had no EventBridge rule, so they never ran on a CloudFormation deployment: finalize_revocations, analytics_collect, reap_stuck_purchases, fire_scheduled_purchases, send_notifications and cleanup. That includes the delayed-purchase fire sweep. The coverage test from #902 listed all six as known cloudformation gaps.

Fix

Each missing task gets one AWS::Events::Rule and one AWS::Lambda::Permission. Each pair is gated by a Schedule<Task> condition on an Enable<Task>Schedule parameter ("true"/"false"), and the schedule comes from a matching <Task>Schedule parameter. The target is LambdaFunction with Input: '{"action": "<task>"}', the same contract the existing rules use. The parameters are listed in a new "Scheduled Tasks" console parameter group.

Task Default Schedule Why
finalize_revocations on rate(1 hour) Not a purchase path.
analytics_collect on rate(1 day) Same default as the Lambda module.
reap_stuck_purchases off rate(5 minutes) The owner decided on #712 that the CFN reap rule ships off. The #894 change has since landed as #1042, but it sets the approved-row floor at 1h20m, which assumes an hourly process sweep. On this stack PurchaseCheckRule runs process only daily, so a reaper would still fail a stranded approval before process re-drives it.
fire_scheduled_purchases off (opt-in) rate(15 minutes) Money path, opt-in per the plan.
send_notifications off (opt-in) rate(1 day) Opt-in per the plan.
cleanup (2nd commit) off (opt-in) rate(1 day) The first run deletes every execution past the 30-day retention.
  • The cadence defaults equal the Terraform Lambda module defaults, and a test pins that.

  • Rule names are ${AWS::StackName}-Finalize, -AnalyticsCollect, -ReapStuck, -FireScheduled, -SendNotifications and -Cleanup. None is longer than the existing -ScheduledExecution, so every stack name the existing rule accepts still fits EventBridge's 64-character limit.

  • The two existing rules are unchanged. That includes the always-on daily PurchaseCheckRule (process), even though the plan's policy makes process opt-in for new schedules: turning it off would stop purchases on existing stacks. The test records it as an explicit exception.

  • scheduled_task_coverage_test.go: the cloudformation entry is removed from knownGaps.

  • scheduled_task_cfn_test.go (new): parses the template with yaml.v3, rewriting short-form intrinsics to their long form. For every required server task (the roster minus exemptTasks), it checks:

    • there is exactly one rule;
    • the rule has a single target, LambdaFunction.Arn, whose Input is exactly {"action": task};
    • exactly one invoke permission has SourceArn set to that rule, the right action, principal and function, and the same Condition as the rule;
    • only the two pre-existing rules are unconditional;
    • the enable parameter allows exactly "true"/"false", and its default follows the opt-in policy (optInRequired/exemptTasks). Exceptions are listed in cfnDefaultExceptions with reasons, and each one is asserted to still apply;
    • the schedule is a Ref to a parameter whose default equals the Lambda module's *_schedule default;
    • the name suffix is short enough and unique, and State is ENABLED.

    A rule that sends an unknown task fails. A new server task fails until it is scheduled here or listed as a CFN gap.

  • Docs (2nd commit): docs/DEPLOYMENT.md and known_issues/33 now describe the opt-in CloudFormation cleanup parameter.

Deploy note

No workflow deploys this template. Customers apply it themselves, so merging deploys nothing. This PR is AWS CloudFormation only. The Azure/GCP deploy pause (since 2026-10-20) does not gate it, and none of its effect waits on that pause. The remaining #712 slices are Fargate, then GCP and Azure; the GCP/Azure ones will wait on the pause.

Verification

All of this is static, fixture and unit evidence. Nothing was deployed and no AWS API was called. cfn-lint and trivy are not installed on this machine, so the template's only machine check here is the yaml.v3 parse plus the structural assertions above. CI's trivy step will scan it.

Fail-before: I put back origin/main's template.yaml with the new test in place.

$ go test -count=1 -run 'TestCloudFormation|TestRequiredTasksWired' ./terraform/modules/compute/
    scheduled_task_cfn_test.go:328: finalize_revocations: want exactly one rule, got []
    scheduled_task_cfn_test.go:328: fire_scheduled_purchases: want exactly one rule, got []
    scheduled_task_cfn_test.go:328: reap_stuck_purchases: want exactly one rule, got []
    scheduled_task_cfn_test.go:328: send_notifications: want exactly one rule, got []
    (and analytics_collect)
    scheduled_task_coverage_test.go:325: cloudformation: undocumented gap: analytics_collect ...
FAIL

The new template with the old knownGaps also fails, with cloudformation: knownGaps lists analytics_collect but it is wired and the same for the other four.

Pass-after, on this branch (head e5bcda0a):

go build ./...                                                    # ok
go vet ./... ; go vet -tags integration ./...                     # ok
go test -count=1 ./terraform/modules/compute/                     # ok
go test -count=1 ./internal/mocks/                                # ok
golangci-lint v2.10.1 run --allow-parallel-runners ./terraform/modules/compute/...   # 0 issues
gofmt -l terraform/modules/compute/                               # clean
bash scripts/check-aws-iam-parity.sh                              # OK (it reads this template)
pre-commit run markdownlint|trailing-whitespace|end-of-file-fixer --files <changed files>   # Passed
  • gocyclo does not apply: no non-test Go files changed.
  • terraform fmt/validate/test do not apply: no Terraform files changed.

Mutation probes are built into TestCloudFormationScheduleCheckNegatives. Each one edits the real template text and must produce a problem. I checked that each fails for its intended reason:

  • a rule removed or its input reassigned;
  • an opt-in task switched on;
  • a default-on task switched off;
  • an exception flipped back to the policy;
  • a permission pointing at another rule;
  • a permission or rule ungated;
  • a name that is too long or reused;
  • a literal schedule;
  • schedule-default drift from the Lambda module;
  • a wrong target ARN;
  • an extra input key;
  • State: DISABLED;
  • an unknown task;
  • an enable condition that is not "true";
  • a pre-existing rule made conditional.

Long-form Fn::GetAtt is covered too.

Overlap: git merge-tree against #1086, #1090, #1203, #1119, #1114 and fork branches fix/issue-1233 and fix/issue-323 merges cleanly. go test ./terraform/modules/compute/ passes on the merge with #1114, which also edits knownGaps, DEPLOYMENT.md and known_issues/33.

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 936996ad = origin/main a14dc8f2 plus these heads, merged in this order:

Branch Tested head
fix/issue-337 ff3420c
fix/issue-169 7d75eb5
fix/issue-342 66b221f
fix/issue-310 5e11c21
fix/issue-326 5f0ea59
fix/issue-243 365e959
fix/issue-343 6c94036
fix/issue-312 d2848a8
fix/issue-316 acd60dd
fix/issue-339 75ea9b9
fix/issue-339-commitment-type 73bdb19
fix/issue-353 5d25fcd
fix/issue-313 3156bc4
fix/issue-357 53d9e58
fix/issue-368 0ba727d
fix/issue-419 61648c9
fix/issue-111 b04c372
fix/issue-686 0ad90b7
fix/issue-674 793095d
fix/issue-1055 4ec96f1
fix/issue-559 dfa9525
fix/issue-558 034eed4
fix/issue-874 1720539
fix/issue-666 1e120da
fix/issue-668 fb26345
fix/issue-568 ce04173
fix/issue-311 e54f250
fix/issue-177 0cbb88a
fix/issue-151 c21377c
fix/issue-166 41fb77b
fix/issue-27 2d67622
fix/issue-323 69288ae
fix/issue-59 36463e8
fix/issue-43 35e4069
fix/issue-158 6f76f8a
fix/issue-104 bb00491
fix/issue-55 971ef1b
fix/issue-361 89ac220
fix/issue-120 b458719
fix/issue-36 42bac6e
fix/issue-525 57c97dd
fix/issue-712 e5bcda0
fix/issue-72 6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; whichever PR merges second takes that same resolution.

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:

  • Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
  • Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
  • Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
    • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
    • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
    • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
  • What the combined run caught, each fixed on its own branch before this PR opened:
    • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
    • fix/issue-316 rejecting legacy-keyed plans;
    • fix/issue-120 versus main's global-filter test.
  • Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns 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: 137 PASS, 0 FAIL.
  • Nothing was applied to any cloud.

Out of scope / for the maintainer

  • The CloudFormation stack cannot run the current server at all. It provisions DynamoDB tables and sets no DB_HOST/DB_* environment. initConfigStore (internal/server/app.go) returns DB_HOST must be set, and cmd/server exits with log.Fatalf. So these rules, like the two existing ones, only do anything once the stack is brought up to the Postgres-backed server. I found no issue tracking this; it probably deserves one, and it may affect how much this slice is worth.
  • docs/DEPLOYMENT.md still says "Scheduling for AWS Fargate, CloudFormation and GCP is tracked by fix(iac): several scheduled tasks are not scheduled; delayed purchases never fire on GCP/Azure #712". I left that line alone because fix(iac/azure): schedule the server tasks on AKS with CronJobs #1114 rewrites it. Dropping "CloudFormation" from it can follow once both have merged.
  • Reap stays off on CFN, per the owner's decision. Turning it on safely would mean also making process hourly here, or giving process an enable/schedule parameter. Changing the existing PurchaseCheckRule is a behavior change for live stacks, so it is not done here.
  • process_scheduled_purchases stays always-on on CFN (pre-existing), unlike the opt-in policy elsewhere.

Please mirror the issue's triage labels on this PR: priority/p1, severity/high, urgency/this-sprint, impact/many, effort/m, type/bug, triaged.

Refs #712

🤖 Generated with Claude Code

IAmJSD and others added 2 commits October 23, 2026 08:10
…asks

Add an EventBridge rule and invoke permission to the CUDly CloudFormation
stack for each scheduled server task it was missing, each switched by an
Enable*Schedule parameter with a matching *Schedule expression:

- finalize_revocations: on, rate(1 hour)
- analytics_collect: on, rate(1 day)
- reap_stuck_purchases: off (owner decision on LeanerCloud#712: the PurchaseCheck
  rule runs process only daily, so the reaper's 1h20m approved floor
  would still fail stranded approvals before process re-drives them)
- fire_scheduled_purchases: off (opt-in), rate(15 minutes)
- send_notifications: off (opt-in), rate(1 day)

Cadences match the Terraform Lambda module defaults. The existing
collection and PurchaseCheck rules are unchanged.

A new Go test parses the template and checks, for every required server
task, one rule targeting the Lambda with {"action": task}, one permission
gated by the rule's condition, an enable default that follows the opt-in
policy (with listed exceptions), a schedule parameter matching the Lambda
module default, and a name suffix no longer than the oldest rule's.
cleanup stays a documented CloudFormation gap until the next commit.

Refs LeanerCloud#712

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add EnableCleanupSchedule (default "false") and CleanupSchedule
(default rate(1 day)) to the CUDly CloudFormation stack, with a
<stack>-Cleanup rule and invoke permission that run the server cleanup
task. It stays opt-in, as on the Lambda module: the first run deletes
every execution past the 30-day retention.

Drop the last CloudFormation entry from knownGaps and document the
parameter in DEPLOYMENT.md and known_issues/33.

Refs LeanerCloud#712

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 23, 2026
@coderabbitai

coderabbitai Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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