[IAmJSD]

Root cause

cmd/cleanup-lambda/main.go (deleteExpired, and the dry-run dryRunCount) selected purchase_executions rows with only expires_at < now. expires_at is written once at insert by SavePurchaseExecution and never rewritten on a status change, so a lapsed TTL says nothing about whether the row is still live. It is latent today, as the issue says (no production code sets PurchaseExecution.TTL, and migration 000124's trigger keeps capacity-holding rows of unfinished ramps), but the first TTL on a live row would let the job delete it. It would also purge failed and canceled rows the plan health score is still counting.

Fix

deleteExpired and dryRunCount now share one predicate, expiredExecutionsWhere:

expires_at < $1 AND status = ANY($2) AND status <> ALL($3)
-- $2 = config.TerminalExecutionStatuses, $3 = config.HealthScoredExecutionStatuses

• TerminalExecutionStatuses is the existing allowlist of rows nothing will act on again (the one DeleteCloudAccount uses). Because it is an allowlist, a status added later is kept until someone classifies it.
• HealthScoredExecutionStatuses (failed, canceled, cancelled) is excluded the same way CleanupOldExecutions excludes it from its expiry branch, so those rows stay with that sweep's retention windows.
• So a lapsed TTL now deletes only completed, partially_completed, expired and revocation_requested rows. Both lists come from internal/config, so the job can't drift from the store. That adds 4 packages to the binary's dependency graph.

Concurrency: the job's DELETE runs under READ COMMITTED. A row it has to wait for is re-checked against the full predicate once the other transaction commits, so a row that stops being settled during the wait survives. A test covers this.

Note on the issue's premise

The issue says CleanupOldExecutions "excludes live statuses". On current main it doesn't. Its branch 3 (status <> ALL(HealthScoredExecutionStatuses) AND expires_at < NOW() - retention) excludes only failed and canceled rows, so it still deletes a pending row with a lapsed TTL. TestPruneRechecksRowAfterCommandCancelsIt ("prune first") asserts exactly that. Copying its predicate as-is would not meet the issue's acceptance test ("a live row with a past expires_at survives"). That is why this PR adds the terminal-status allowlist on top. I left CleanupOldExecutions alone because changing it is outside this issue's scope and would change #1014's race test. It has the same latent gap, so a maintainer may want a follow-up issue for it.

Verification

All local, against a real Postgres 16 testcontainer with every migration applied. This is DB integration evidence, not real-cloud: no Lambda was deployed or invoked, and no AWS API was called.

New file cmd/cleanup-lambda/main_integration_test.go (//go:build integration, so CI's integration job runs it):

• TestCleanupStatusListsCoverSchema: every status in the purchase_executions status CHECK is classified exactly once as deletable or kept, so a new status fails this test until someone decides.
• TestDeleteExpiredKeepsLiveAndHealthScoredRows: one row per status with a lapsed TTL, plus future-TTL and NULL-TTL rows for each deletable status. The dry run counts exactly 4, the delete removes exactly those 4, and every other row survives.
• TestDeleteExpiredRechecksRowAfterConcurrentWriter: both orders. "writer first": a transaction turns a lapsed completed row back to pending and holds the lock. The cleanup DELETE is shown waiting on it via pg_stat_activity, then the writer commits, and the row survives with 0 deleted. "cleanup first": the row is deleted and the writer's UPDATE affects 0 rows. The writer is a stand-in: SavePurchaseExecution's upsert rewrites status unconditionally, but no current flow moves a settled row back to live.

AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null \
  go test -tags integration -race -count=1 -v ./cmd/cleanup-lambda

• After the fix: all three tests pass (ok ... 7.765s).
• Before the fix (main's main.go with the new test): TestDeleteExpiredKeepsLiveAndHealthScoredRows fails (expected: int(4) actual: int64(13)), and TestDeleteExpiredRechecksRowAfterConcurrentWriter/writer_first fails (Should be zero, but was 1). cleanup_first and the schema-coverage test pass, as expected.
• Mutation probes, each caught:
  • dropping status = ANY($2) fails both behavior tests;
  • dropping status <> ALL($3) fails the status test;
  • reverting only the dry-run query fails the status test;
  • reverting only the DELETE query fails both behavior tests.

Other checks run locally:

• go build ./...
• go vet ./cmd/cleanup-lambda and go vet -tags integration ./cmd/cleanup-lambda
• golangci-lint v2.10.1 on ./cmd/cleanup-lambda/..., with and without --build-tags integration: 0 issues
• gocyclo -over 10 -ignore "_test\.go" cmd/cleanup-lambda: silent
• go test ./internal/mocks/ ./internal/database/postgres/testhelpers/ -count=1: ok
• scripts/gosec-hook.sh (gosec v2.28.0) on the changed files: clean
• markdownlint-cli@0.47.0 on the known_issues doc: clean
• git-secrets scan of the changed files in an isolated throwaway repo, plus scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL
• Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid. The tests don't touch AWS.

Also in this PR / out of scope

• known_issues/35_legacy_guard_followups.md: the fix(cleanup): give the cleanup lambda the live-status guard CleanupOldExecutions has #998 entry is marked resolved in place. The file stays where it is because feat(plans): settle a failed-with-submission row so a managed plan can be deleted #999 is still open.
• .gitignore line 90 (cleanup-lambda, meant for the compiled binary) also matches the cmd/cleanup-lambda/ directory, so the new test file had to be force-added, as main.go evidently was. I didn't change .gitignore; anchoring it (e.g. /cleanup-lambda) would be a separate tidy-up.
• CleanupOldExecutions branch 3 still deletes live rows with a lapsed TTL; see the note above.

Maintainer: please mirror the issue's triage labels onto this PR: triaged, priority/p3, severity/low, urgency/eventually, impact/few, effort/s, type/bug.

Closes #998

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 37a40f1
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1a7667b2-8060-418d-bb32-94f921d20219

📥 Commits

Reviewing files that changed from the base of the PR and between 37a40f1 and 78fdd72.

📒 Files selected for processing: 3

• cmd/cleanup-lambda/main.go
• cmd/cleanup-lambda/main_integration_test.go
• known_issues/35_legacy_guard_followups.md

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[cristim]

VERDICT: APPROVE 78fdd72

PR #1173 (closes #998), one commit, base origin/main cb25d99. git merge-tree --write-tree origin/main 78fdd72d is clean. Largest commit: 78fdd72, 214 changed lines (207+/7-), under the 400 cap.

Findings

• nit cmd/cleanup-lambda/main.go:30-32 - the comment says expires_at "is written once at insert by SavePurchaseExecution and never rewritten when the row changes status". The upsert does rewrite it: internal/config/store_postgres.go:1106 (expires_at = $13), fed by timeFromTTL(execution.TTL) at :1159. What holds is that load-then-save round-trips the stored TTL (store_postgres.go:2040), so a status change leaves it as it was. The conclusion (a lapsed TTL says nothing about liveness) is right; the stated mechanism is wrong. The PR body repeats the claim.
• nit (pre-existing, not a regression) cmd/cleanup-lambda/main.go:52 - expired rows are deletable with no retention window. config.RampStepStuckStatuses (internal/config/types.go:502) counts expired rows for the ramp_blocked health factor, and ramp_ledger_holds (migration 000124) does not protect them. If a TTL is ever set, the lambda could clear a stuck-step signal sooner than CleanupOldExecutions' branch 3, which keeps such rows until NOW()-retention. Main deleted these rows too, so this is a possible follow-up, not a blocker.

The issue's premise note in the PR body is accurate: CleanupOldExecutions branch 3 excludes only HealthScoredExecutionStatuses. Adding the TerminalExecutionStatuses allowlist on top is the right call, because copying branch 3 alone would fail the issue's acceptance test.

Attacked:

• Status coverage: the CHECK in migration 000089:101-107 (13 values, no later migration changes it) against TerminalExecutionStatuses (types.go:566-574) and HealthScoredExecutionStatuses (types.go:538-542). Deletable = completed, partially_completed, expired, revocation_requested. Kept = pending, notified, approved, running, scheduled, paused (in flight or awaiting approval), plus failed, canceled, cancelled. revocation_requested is settled: no code moves a row out of it (types.go:544-547; handler_purchases.go:1970 is the only writer). No pending-approval or in-flight status can match status = ANY(Terminal).
• Ramp trigger: retain_ramp_ledger_execution returns NULL (a silent skip, not RAISE), so a protected row cannot abort the DELETE transaction.
• Time zones: expires_at is TIMESTAMPTZ (000001:92), so the comparison is instant-based. Re-ran the behavior test under TZ=Pacific/Kiritimati: pass.
• Concurrency: READ COMMITTED re-check, exercised by the writer-first subtest.
• CI coverage: ci.yml:466 runs go test -tags=integration ./... on the root module, so the new file runs in CI (the .gitignore match does not drop the tracked file).

Local results (fresh scratch export of 78fdd72, Postgres testcontainer, under lockf heavy.lock):

• go build ./... OK; go vet ./cmd/cleanup-lambda/ and go vet -tags integration ./... OK.
• go test -tags integration -race -count=1 ./cmd/cleanup-lambda/: all 3 tests pass (9.9s).
• Mutation probes, each made and then reverted (file cmp-restored):
  • M1, drop status = ANY($2): the keep test fails (4 vs 10) and writer_first fails.
  • M2, drop status <> ALL($3): the keep test fails (4 vs 7).
  • M3, revert only the dry-run query: the keep test fails (4 vs 13).
  • M4, flip expires_at < to >: both behavior tests fail.
• CI on the head: all checks pass (Snyk skipped).

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 78fdd72

Findings (none blocking):

• [non-blocking, latent] cmd/cleanup-lambda/main.go:52 (expiredExecutionsWhere) - expired rows stay deletable once their TTL lapses, but expired is in config.RampStepStuckStatuses (internal/config/types.go), which GetStuckRampSteps uses to raise the plan-health ramp_blocked factor. The ramp_ledger_holds trigger (migration 000124) does not protect expired rows. So once TTLs are set, deleting an expired row on the next step of an unfinished ramp would clear a ramp_blocked signal without anyone acting on it. This is latent, since no production writer sets a TTL. CleanupOldExecutions branch 3 has the same exposure, and it is no worse than base (base deleted every status). It fits the follow-up the PR body already suggests.
• [non-blocking, evidence wording] The PR body's mutation claims are overstated. Dropping status <> ALL($3), status = ANY($2) or expires_at < $1 outright does fail the tests, but only because pgx and Postgres reject the leftover parameter. Re-run as semantic no-ops that keep the parameter, all three are still caught (S1-S3 below), so the tests are sound.

Against the issue: it asks for "the same predicate as CleanupOldExecutions". That predicate's branch 3 (internal/config/store_postgres.go:2264-2268) excludes only health-scored statuses and would still delete pending. The extra TerminalExecutionStatuses allowlist (types.go:566) is what meets the issue's acceptance test, and it fails closed for new statuses. The root cause is fixed.

Attacked:

• FK and trigger fallout: retry_execution_id and ladder_tranches.execution_id are ON DELETE SET NULL (000042/000044/000081). The ramp retain trigger returns NULL without raising, so a protected row cannot abort the shared sessions+executions transaction.
• Nulls: status is NOT NULL, so <> ALL is safe. A NULL expires_at never matches, and a test covers it.
• Blast radius: no exported symbol changed. origin/main's main.go equals the base, so there is no drift.
• Probes on a scratch copy, each restored and confirmed identical with cmp:
  • P4, the dry-run query reverted: FAILS.
  • P5, Terminal and HealthScored args swapped: both behaviour tests FAIL.
  • S1, status <> ALL($3) replaced with ($3::text[] IS NOT NULL): FAILS.
  • S2, status = ANY($2) replaced with ($2::text[] IS NOT NULL): FAILS.
  • S3, the TTL clause made always-true: FAILS.
  • P6, < changed to <=: passes, an acceptable boundary gap at hour offsets.

Verified locally, on a git archive export of HEAD with no cloud credentials:

• go build ./... and go vet ./cmd/cleanup-lambda, with and without -tags integration: clean. gofmt -l: clean.
• go test -tags integration -race -count=1 ./cmd/cleanup-lambda, under the heavy lock: 3/3 PASS.
• Fail-before, on the base export: KeepsLive FAILS (expected 4, actual 13), and writer_first FAILS.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Local verification at 78fdd72d55d53b0d7ec30a35e643b8911476ad34 merged onto current main (a7c2ff29; merge-tree 70e64655; macOS):

• go build ./..., go vet ./... and go vet -tags integration ./... pass.
• go test -race -short -count=1 on the packages this PR changes passes: ./cmd/cleanup-lambda.
• The full go test -race -short ./... suite passes on the tree with every PR in this merge train applied (tree 99ee50e2).
• Nothing was applied to any cloud.

[IAmJSD]

Marked ready and merged by IAmJSD's agent, at IAmJSD's request, after 2 independent reviews and local verification (see the review and verification comments above, all at 78fdd72d55d53b0d7ec30a35e643b8911476ad34).