Skip to content

fix(purchase): make partially_completed runs revocable and emailed; pin the approved-row single claimer - #1257

Draft
IAmJSD wants to merge 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-532
Draft

IAmJSD wants to merge 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-532

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Slices 2 and 4 of the #532 plan (plan comment, checked against main feec673c). Slice 1 merged as #644. This PR leaves slice 3 (token-mode mapping) open, so it says Refs, not Closes.

Refs #532

Status of each plan item on current main (a14dc8f2)

Plan item Status on main This PR
Session approve maps every error to 409 Done in #644 (executeFailureError) Nothing to do
Direct execute maps every error to 409 and stamps executed_at before the claim Done in #644 (Manager.DirectExecute stamps after the CAS) Nothing to do
Token approve: failures after the claim are a raw 500, not 502 with the row status Still present. ApproveExecution returns no row and tokenActionError falls through to the raw error Not done (slice 3). The open #1242 rewrites ApproveExecution / transitionApproveAndExecute (token pin in the CAS). Changing the same signature here would conflict, so slice 3 should follow #1242. This PR only adds the partial-run email on that path (below).
Partial runs mint no revocation token and send no executed email Still present. transitionApproveAndExecute returned before rotateApprovalToken on any execErr Fixed (slice 2)
Approved-row claim race Handled in code since #833 (issue 829): every synchronous run CASes straight to running, claimAndRedrive claims only approved, and TestSingleClaim_* covers cron/SQS. Not tested yet: the stale-approved re-drive arriving during a sync run Pinned with a test (slice 4)

Root cause (slice 2)

executeAndFinalize returns the partial sentinel for a run that bought some commitments and saved the row partially_completed. transitionApproveAndExecute treated that like any other failure: it returned (row, "", err) before minting the revocation token. The handlers then answered 502 without the executed email. So a partially bought purchase had no revoke link, and the approver got no executed email. The row also kept its pre-approve approval-token hash.

Fix

  • purchase: a new IsCommittedPartialRun(row, err) is true when the row is partially_completed and the error is not an audit loss. On an audit loss the stored row is still running. The tail of transitionApproveAndExecute moved into runClaimedAndMint, which also mints the token for a committed partial run and returns it with the run's error. Completed runs, failed runs and audit losses behave as before.
  • api: session approve, direct execute and run-now go through a new executeFailure. For a committed partial run it sends the executed email with the row the funnel returned and the minted token, then returns the same executeFailureError mapping. That is still 502 with {execution_id, status: "partially_completed"}, keeping the 502-for-partial decision from fix(purchase): map session approve and direct execute failures by cause #644. The message now reads "partially completed: some commitments were purchased and can be revoked, others failed".
  • Token approve (approveViaToken): ApproveExecution already passes the funnel's token through, so a token returned with an error marks a committed partial run. The handler emails it from the re-fetched row (the re-fetch moved into emailTokenApproveOutcome). The response mapping is unchanged (slice 3).
  • Executed email: new NotificationData.PartialPurchase. For a partial run, the subject, text and HTML say "partially executed" / "Not every commitment in this purchase went through", instead of "N commitment(s) were purchased successfully". For a single-account partial row, the email lists only the recs marked Purchased, with totals from those recs. A multi-account root has no per-rec Purchased flags, so it lists every rec under the partial notice.
  • openapi.yaml: the approve and execute 502 descriptions now say a partially_completed run is revocable and emailed.

No migration. No frontend change.

Slice 4 (approved-row claim race)

Since #833, the single claimer is the CAS to running. TestSingleClaim_StrandedRedriveMidSyncRunCannotBuyAgain replays the remaining case from the plan on real PostgreSQL, once each for approve, direct execute and run-now. During the sync run's provider call, a claimAndRedrive that saw the row as approved (a stale listing) tries to claim it. It must lose and buy nothing, and the row must end completed after exactly one provider call. This test passes on main (it pins #833) and fails under a mutation that lets the re-drive claim running rows.

Verification

All local, Linux, Go 1.26.9. The integration tests use Docker testcontainers PostgreSQL with the repos' fake/mock providers and recording email notifiers. Nothing calls a cloud API. All tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null.

Fail before, on origin/main a14dc8f. I copied the new integration tests into a detached worktree of main. Assertions on symbols that don't exist there yet (IsCommittedPartialRun, PartialPurchase, the rec list) were removed from the copy.

  • go test -tags integration -count=1 -run 'TestApproveAndExecute_PartialRunMintsRevocationToken|TestApproveAndExecute_FailedRunMintsNoToken|TestSingleClaim_StrandedRedriveMidSyncRunCannotBuyAgain' ./internal/purchase/ failed with TestApproveAndExecute_PartialRunMintsRevocationToken: Should NOT be empty ... a partial run is revocable, so it needs a revocation token. The failed-run control and the slice-4 pin pass on main, as expected.
  • go test -tags integration -count=1 -run TestRunNow_PartialRunEmailsRevocationToken ./internal/api/ failed with Not equal ... a partial run must send the executed email (0 calls).
  • The slice-4 pin under the mutation claimAndRedrive claiming {"approved","running"} fails in all three subtests.

Pass after, on this branch.

  • The same purchase integration tests: all pass (69s). This includes a multi-account fan-out root ending partial: the stored hash matches the returned token, with expiry inside RevocationWindow.
  • go test -tags integration -count=1 -timeout 20m -run 'TestRunNow_' ./internal/api/: ok. The new test runs a real run-now through the handler, real manager and real store. One of two purchases fails. The test checks the 502, a single executed email, that the emailed token passes validateRevokeToken against the stored row, the expiry window, PartialPurchase, and that only the bought rec is listed.
  • Unit tests: go test -count=1 on ./internal/email/, ./internal/purchase/, ./internal/api/ and ./internal/mocks/: all ok.
  • Mutation probes on the new unit tests (each reverted):
    • Drop the email in executeFailure: the session and direct partial tests fail.
    • Make the token path return early on any error: the token partial test fails.
    • List all recs for a partial: three partial tests plus TestExecutedEmailCommitments fail.
    • Drop PartialPurchase: the three partial tests fail.
    • Drop the audit-loss exclusion from IsCommittedPartialRun: UncommittedFailureSendsNoEmail/partial_audit_loss fails.
  • go build ./..., go vet ./... and go vet -tags integration ./...: clean. golangci-lint v2.10.1 on internal/api, internal/purchase and internal/email, with and without --build-tags integration: 0 issues. gocyclo -over 10 on the changed non-test files: silent. gofmt -l: clean.
  • Merge simulation with the open sec(purchase): pin the approval token in the token approve/cancel CAS; compare revoke token first #1242 (git merge-tree): no conflicts. The combined tree builds, go vet -tags integration passes on internal/purchase and internal/api, and the unit tests for both packages pass.

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

Branch Tested head
fix/issue-47 62feb4f
fix/issue-76 3b8c241
fix/issue-1233 e1638b7
fix/issue-153 8729074
fix/issue-532 0971624
fix/issue-64 fefdc43
fix/issue-54 91827b9

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: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
  • Static checks (changed packages): golangci-lint v2.10.1 found 0 issues, with and without --build-tags integration, and gocyclo -over 10 is silent on the changed non-test files. These ran at b61a9c30, which differs from 77d116c6 only by one .gitallowed line on fix/issue-1233.
  • Integration tests, every changed package, one package at a time (-tags integration -count=1):
    • These packages pass: cmd/cleanup-lambda, internal/analytics, internal/auth, internal/database/postgres/migrations and internal/purchase.
    • internal/api passes, except two tests whose Postgres container didn't start, or whose migrations hit a timeout, under machine load. Rerun alone on the same tree, both pass: TestDeleteManagedPlanByExecutionStatus and TestNotifiedRow_RescheduleDoesNotOverwriteConcurrentCancel.
    • 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).
    • Packages were run with a 60-minute timeout (rather than CI's 20) because the VM was shared with other test runs. Some reruns used TESTCONTAINERS_RYUK_DISABLED=true to avoid a testcontainers reaper race under load.
  • 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 / notes

  • Slice 3 (token-mode mapping) is still open. ApproveExecution should return the row so that post-claim failures on the email-link/SQS approve map to 502 with the row status. This should land after sec(purchase): pin the approval token in the token approve/cancel CAS; compare revoke token first #1242, which changes the same signature. Until then, a partial token approve sends the email but still answers 500.
  • On a partial token approve whose best-effort mint failed, there is no token to signal the partial run, so no executed email is sent. The session paths still send it, without the revoke panel. Slice 3's row return removes this gap.
  • Pre-existing, not changed: rotateApprovalToken writes the token with a full-row SavePurchaseExecution right after the terminal save. A session revoke landing in that window would be overwritten back to the terminal status. This already applied to completed runs and now also applies to partial ones. A status-guarded save (SaveExecutionIfStatus on the terminal status) would close it, but it touches many mock expectations.
  • Pre-existing, not changed: on success, session approve and direct execute email the pre-approve snapshot. It has no completed_at, so the email's "revoke until" line is blank. The partial path emails the returned row, which has it.
  • The SQS approve worker sends no executed email for any outcome. That is unchanged.

Maintainer: please mirror the issue's triage labels on this PR: priority/p2, severity/medium, urgency/this-quarter, impact/few, effort/l, type/bug, triaged.

🤖 Generated with Claude Code

IAmJSD and others added 2 commits October 23, 2026 04:30
…r partial runs

A run that ended partially_completed bought part of its commitments, but
the approve funnel returned before minting the revocation token, so the
row could not be revoked by link and nobody got the executed email.

transitionApproveAndExecute now mints the token for a saved
partially_completed run too (IsCommittedPartialRun: partial status and no
audit loss) and returns it with the run's error. Session approve, direct
execute and run-now send the executed email for such a run before the
502, and the 502 message says the run partially completed. The token
approve emails it when ApproveExecution returns a token with its error;
its status mapping is unchanged. The executed email marks a partial run
and lists only the commitments it bought when the row records them.

Refs LeanerCloud#532

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…flight sync run

The stale-approved recovery (claimAndRedrive) claims only "approved" rows,
and approve, direct execute and run-now hold their row "running" for the
whole provider call (LeanerCloud#829), so the CAS to "running" is the single claimer.
Replay the race on real PostgreSQL: a re-drive that read the row as
approved arrives mid-run, loses the claim and buys nothing.

Refs LeanerCloud#532

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added priority/p2 Backlog-worthy severity/medium Moderate harm effort/l Weeks triaged Item has been triaged urgency/this-quarter Within the quarter impact/few Limited audience 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.

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/l Weeks impact/few Limited audience priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant