Skip to content

fix(purchase): pin each execution lineage's fan-out shape - #896

Merged
cristim merged 3 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-97
Oct 16, 2026
Merged

cristim merged 3 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-97

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

Root cause. The single-account path derives per-rec provider idempotency tokens from the lineage key K; the per-account fan-out derives them from K:<accountID>. executePurchase picked the path from the plan's live plan_accounts, which is mutable configuration and not recorded on the row. A plan that gained its first account (or lost its last one) between an attempt and its Retry therefore derived tokens the provider had never seen, so a commitment the first attempt bought (e.g. a reaped run whose purchase landed) was bought again.

Fix. The first attempt of a lineage pins the shape it runs with (single / multi) in a new purchase_execution_fan_out_shapes table keyed by the lineage key (idempotencyLineageKey, so it spans Retry successors with no row-copying). The write is first-writer-wins in one statement (INSERT ... ON CONFLICT DO UPDATE SET shape = shape RETURNING shape), and the row is never updated. Every later attempt follows the pin:

Pinned Plan accounts now Behavior
matches live either unchanged (the same path and tokens as before)
single ≥ 1 re-drives the single-account path, so it re-derives Derive(K, i) and the provider dedupes
multi 0 refused before any provider call, with an actionable message (re-attach accounts or cancel)
pin write fails either refused before any provider call (fail closed)

Keys with a colon (scopeless per-account rows, #1537) and empty keys skip the pin, and reattachAccountScope still handles them as before.

Backward compatibility. A lineage whose plan never crosses zero-to-N takes the same path as before, so its tokens are byte-identical. The tests pin Derive("lineage-97", 0) and Derive("lineage-97:acct-A", 0) on both attempts.

Migration / in-flight executions. Migration 000123 (additive and idempotent; the down migration drops the table) creates the table and backfills multi for every root lineage that already has a per-account (K:A) row, because only the fan-out writes one. It deliberately does not infer single from the absence of per-account rows: a fan-out reaped before any per-account row was saved leaves none. Remaining gap: an in-flight lineage with no per-account row whose plan already crossed zero-to-N before this deploys gets pinned to the live shape on its next attempt, the same as today. If operators know of such retries pending, they should check the provider-side state before approving them.

Notes for review.

  • A lineage whose first attempt failed before submission (e.g. bad credentials) still pins its shape. If accounts are attached later, its retries keep the single-account path, which resolves credentials from the recs' account, the same as the first attempt.
  • Pin rows aren't cleaned up when executions are deleted. They are tiny and harmless.
  • A fan-out whose account set changes (N to M, both > 0) is out of scope. That is the companion per-account retry fan-out issue.

Closes #97

Verification

All of this ran locally on macOS (Go 1.26.9) at 766dc538. No real cloud calls were made: the provider chain is mocked, and integration tests use a testcontainers Postgres.

Regression tests fail before the fix (internal/purchase/fan_out_shape_test.go, run on unmodified origin/main + test file only):

go test ./internal/purchase/ -run 'TestRetryAfterPlan|TestUnchangedPlanKeepsItsTokens' -count=1 -v
--- FAIL: TestRetryAfterPlanGainsFirstAccountReusesSingleAccountToken
    expected: []string{"0ec7ba72e11dd4e251358e62af7a6312f06a08c787b3f4cea09dcabbe1544f33"}   # Derive("lineage-97",0), attempt 1
    actual  : []string{"dc531e28081e62c4b567221ed7234436fb1741bc535a160f78def448947d39aa"}   # Derive("lineage-97:acct-A",0), the retry double-buys
--- FAIL: TestRetryAfterPlanLosesLastAccountBuysNothing
    An error is expected but got nil.   # the retry bought under Derive("lineage-97",0)
--- PASS: TestUnchangedPlanKeepsItsTokensAcrossRetries (both subtests)   # back-compat control

and pass after it:

--- PASS: TestRetryAfterPlanGainsFirstAccountReusesSingleAccountToken
--- PASS: TestRetryAfterPlanLosesLastAccountBuysNothing
--- PASS: TestUnchangedPlanKeepsItsTokensAcrossRetries (both subtests)
--- PASS: TestFanOutShapePinFailureBuysNothing

Other checks:

  • go build ./...: OK
  • go test ./... -count=1: all pass. One test, internal/server TestWireLadderWriteSide_ExecutionDisabled_RealAWSLadder_RefusesWithoutAWSCall, fails only because of this machine's ~/.aws/config (source_profile requires role_arn). It passes with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./internal/server/, and it isn't related to this change.
  • go vet ./... and go vet -tags=integration ./...: OK
  • golangci-lint config verify && golangci-lint run --timeout=10m ./... with the CI-pinned v2.10.1 (built with Go 1.26.9): 0 issues.
  • Integration tests on a real Postgres (testcontainers):
    • go test -tags=integration ./internal/database/postgres/migrations/ -run 'TestMigration_PurchaseExecutionFanOutShapes|TestMigrationsAreTransactional': PASS. Covers the backfill (multi only for lineages with a per-account row), idempotent up, the CHECK constraint and the down migration.
    • go test -tags=integration ./internal/config/ -run TestPinExecutionFanOutShape: PASS. Covers first writer wins, concurrent first callers agreeing, and invalid input.
    • go test -tags=integration ./internal/purchase/: PASS. This is the whole package, including the end-to-end plan-step money-path fixtures.

Not covered: no real AWS, Azure or GCP account was used, and no end-to-end Retry through the HTTP handler against a deployed stack. Provider-side dedupe on a reused token is assumed from the existing #1012 contract.

Triage labels: the closing issue carries triaged, priority/p1, severity/high, urgency/this-quarter, impact/many, effort/m and type/bug. A maintainer should mirror these onto this PR, because the author doesn't have label permissions.

🤖 Generated with Claude Code

The single-account path derives provider idempotency tokens from the
lineage key and the per-account fan-out from "<lineage key>:<account id>",
but the path was chosen from the plan's live plan_accounts. A plan that
gained its first account, or lost its last one, between an attempt and
its Retry derived tokens the provider had never seen and bought again.

The first attempt of a lineage now pins its shape in the new
purchase_execution_fan_out_shapes table (first writer wins) and later
attempts follow the pin: a single-account lineage re-drives the
single-account path, and a fanned-out lineage whose plan has no accounts
left is refused. Lineages that never cross keep byte-identical tokens.
Migration 000123 backfills "multi" for lineages that already have a
per-account row.

Closes LeanerCloud#97

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

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 92782cab-8983-40b5-af3f-ce4b82d2ff0a
📥 Commits

Reviewing files that changed from the base of the PR and between e003025 and dec4b31.

📒 Files selected for processing: 13
  • internal/analytics/collector_test.go
  • internal/config/interfaces.go
  • internal/config/store_postgres_fan_out_shape.go
  • internal/config/store_postgres_fan_out_shape_integration_test.go
  • internal/database/postgres/migrations/000123_purchase_execution_fan_out_shapes.down.sql
  • internal/database/postgres/migrations/000123_purchase_execution_fan_out_shapes.up.sql
  • internal/database/postgres/migrations/000123_purchase_execution_fan_out_shapes_test.go
  • internal/mocks/fan_out_shape.go
  • internal/mocks/stores.go
  • internal/purchase/execution.go
  • internal/purchase/fan_out_shape.go
  • internal/purchase/fan_out_shape_test.go
  • internal/server/test_helpers_test.go
+--------------------------------------------------------------+
| Security by obscurity? I'm about to become very 'unobscure'. |
+--------------------------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@cristim cristim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review of 766dc53: draft: blocks ready

The mechanism is sound for the double-buy scenario in #97: the pin is atomic, keyed by the lineage key that Retry successors copy verbatim, and both directions fail before the fix and pass after. The findings below are about recoverability, one test gap and gate housekeeping.

Findings

  1. Actionable. internal/purchase/fan_out_shape.go:41-45 (called from internal/purchase/execution.go:81). The pin is written before any pre-submission check runs, so a first attempt that never reached a provider still fixes the shape for the whole lineage. Scenario: plan has no accounts, the step's recs span two cloud accounts. Attempt 1 pins single, then resolveSingleAccountProvider refuses it (#1902, execution.go:395-398) with nothing bought. The operator attaches both accounts so the step can fan out and clicks Retry. The successor reads single, takes the single-account path and is refused again, on every retry. Before this PR that retry fanned out. The same applies to a credential-resolution failure. Nothing was ever submitted, so no token needs protecting. Suggested fix: pin lazily, immediately before the first provider submission of the attempt (or let a retry whose predecessor stamped no_submission replace the pin). The PR body lists this under "Notes for review" but treats it as acceptable; it makes the documented remediation ("attach accounts") a dead end for these rows.

  2. Actionable (test gap). internal/purchase/fan_out_shape.go:38. Mutation probe: changing the guard to key == "" (dropping the strings.Contains(key, ":") skip) leaves go test ./internal/purchase/ green. Nothing asserts that a per-account (K:A) or scopeless colon key is never sent to PinExecutionFanOutShape. A regression there would write per-account keys into the pin table and could pin a wrong shape for a re-driven child. Add a test that runs a scopeless K:acct-A row and asserts no pin call is made.

  3. Nit. fan_out_shape.go:51,53,59,62. The multi-no-accounts refusal and the pin-failure refusal are plain errors, not beforeSubmission(...), so the rows are not stamped no_submission. This matches the existing GetPlanAccounts error and reattachAccountScope refusal, so it is consistent, but both refusals provably happen before any provider call. Stamping them would keep the retry gates from treating them as possibly-submitted. Decide deliberately.

  4. Nit. Down migration 000123...down.sql drops the table, so pinned single rows are lost and a re-apply of up only restores multi (backfill by design). Acceptable, but worth a line in the migration header.

  5. Gate housekeeping (actionable before ready). The PR carries no labels; #97 has triaged, priority/p1, severity/high, urgency/this-quarter, impact/many, effort/m, type/bug. Mirror them. The diff is 541 added lines (about 200 production, about 335 test/mock/migration test), over the 400-line cap; consider splitting the migration plus store from the executor change, or note the exception.

Not findings (refuted): migration number 000123 follows main's latest (000122) and no other open PR touches 000123; keys are server-minted UUIDs (store_postgres.go:1016-1017, handler_purchases.go:2192-2204), never client input, so no cross-tenant pin collision; the upsert (ON CONFLICT ... DO UPDATE SET shape = <existing>) takes the row lock and serializes concurrent first callers; the multi backfill of split_part(key,':',1) only matches keys written by executeForAccount.

What I attacked

  • go build ./...; go test of internal/purchase, internal/config, internal/mocks, internal/analytics at the SHA: pass. go vet of purchase and config: clean.
  • Fail-before: restored internal/purchase/execution.go from origin/main with the PR's tests: TestRetryAfterPlanGainsFirstAccountReusesSingleAccountToken, TestRetryAfterPlanLosesLastAccountBuysNothing and TestFanOutShapePinFailureBuysNothing fail; TestUnchangedPlanKeepsItsTokensAcrossRetries passes (back-compat control). All pass at the head.
  • Mutation probes in fan_out_shape.go: disabling the pinned-single branch fails the zero-to-N test; returning !live on a match fails three tests; dropping the colon guard survives (finding 2); retargeting the pinned-multi case to a dead value survives only because the default branch also errors (equivalent mutant).
  • Store: changing DO UPDATE SET shape = <existing> to EXCLUDED.shape fails TestPinExecutionFanOutShape (first-writer-wins and concurrent subtests), so the no-overwrite semantics are covered.
  • Real Postgres (throwaway container): TestMigration_PurchaseExecutionFanOutShapes, TestMigrationsAreTransactional and TestPinExecutionFanOutShape pass at the SHA (backfill, idempotent up, CHECK constraint, down).
  • Read every other GetPlanAccounts caller (plan_step_attach.go:147, api/scoping.go:262, api/handler_plans.go:532): none derive purchase tokens, so executePurchase is the only path-selection point. Retry key propagation (handler_purchases.go:2192-2204) matches the pin key.
  • Not run: internal/purchase integration-tag suite and the full go test ./...; no real-cloud or HTTP Retry end-to-end (none was possible or claimed).

Gate status (origin/main Review gate)

  • Independent review of the full diff naming the SHA: this comment (draft verdict, findings open).
  • Actionable findings resolved: no (1, 2, 5 open).
  • CI on the SHA: not green. CI - Build & Test and pre-commit are action_required (fork workflow approval pending); CodeRabbit skipped the draft. No CR threads exist.
  • Local real-path verification evidence on the PR: present in the PR body (author-run, macOS), and my reruns agree with it; a retry-after-pre-submission-refusal scenario (finding 1) is not covered.
  • Verdict recorded on the PR with SHA: this comment.

Blocks ready

  • Fix or explicitly accept finding 1 (pin only once a submission is possible, or let no_submission retries re-pin).
  • Add the colon-key no-pin test (finding 2).
  • Mirror the labels from #97.
  • Get CI approved and green on the final SHA, then ping CodeRabbit once out of draft.
  • Any new commit restarts this gate.

@cristim cristim added priority/p1 Next up; this sprint severity/high Significant harm urgency/this-quarter Within the quarter impact/many Affects most users effort/m Days type/bug Defect triaged Item has been triaged labels Oct 15, 2026
@cristim

cristim commented Oct 15, 2026

Copy link
Copy Markdown
Member

Clarification of the review above (766dc53), after maintainer calibration. Only these block ready:

  1. The fan-out shape is pinned before the pre-submission checks (internal/purchase/fan_out_shape.go:41-45), so a refused first attempt locks later retries onto the refused path. Pin just before the first provider submission, or let no_submission retries re-pin.
  2. Labels, now mirrored from fix(purchase): a plan crossing zero-to-N cloud accounts between attempts silently changes every idempotency token #97.

Non-blocking: the surviving colon-guard mutation (no test that per-account keys are never pinned) and the size over the 400-line guideline are follow-ups or nits.

Address review of LeanerCloud#896. An attempt refused before any provider call
(e.g. the single-account path refusing recs that span accounts, or a
credential failure) pinned its shape anyway, binding every retry to a
path the operator may have reconfigured the plan to avoid.

The path decision now only reads the pin. The pin is written right
before submission: after resolveSingleAccountProvider on the
single-account path, and after each account's credential resolution on
the fan-out (always under the root lineage key). A concurrent attempt
that pinned the other shape first makes this attempt refuse.

The no-accounts refusal and the pin read/write failures on the
single-account path are stamped beforeSubmission. Adds tests for the
reviewer's scenario, colon keys never reaching the pin store, and the
concurrent-pin refusal, and documents the lossy down migration.

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

IAmJSD commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator Author

Reply to the independent review of 766dc53. The new head is 5dc755f (one new commit, no force-push).

  1. Fixed in 5dc755f. The pin is now written only when the attempt can actually submit:
    • The path decision only reads the pin, through the new GetExecutionFanOutShape. If nothing is pinned, it uses the live plan accounts.
    • The pin is written just before submission. On the single-account path that is right after resolveSingleAccountProvider succeeds, so a #1902 or credential refusal pins nothing. On the fan-out it is inside executeForAccount, after that account's credentials resolve, and always under the root lineage key.
    • If the pin comes back with the other shape, a concurrent attempt submitted first, so this attempt refuses before any provider call.
    • Regression test TestRetryAfterPreSubmissionRefusalFollowsReconfiguredPlan runs your scenario: no plan accounts and recs spanning acct-A and acct-B. Attempt 1 is refused with nothing pinned. Both accounts are then attached, and the retry fans out and buys under Derive(K:acct-A,0) and Derive(K:acct-B,0). I also added TestConcurrentAttemptPinnedOtherShapeBuysNothing for the losing side of a race.
  2. Fixed in 5dc755f. TestColonLineageKeysAreNeverPinned runs a scopeless root-lineage:acct-A row twice: once with the account still on the plan (the reattach path) and once with no plan accounts (the single-account path). Each run asserts that neither GetExecutionFanOutShape nor PinExecutionFanOutShape is called. A third subtest asserts that a root fan-out pins only the key lineage-97. The check now lives in fanOutPinKey. Mutation probe: making the guard key == "" turns both scopeless subtests red. (Your literal edit leaves strings unused and fails to build, so I ran a logically equivalent mutant.)
  3. Fixed in 5dc755f, but only where the claim is provable:
    • The pinned-multi-with-no-accounts refusal, a failed pin read, and the single-account pin failure and concurrent-pin refusal are now beforeSubmission(...).
    • This releases only the refused row's own suppressions. The Retry handler writes fresh suppressions per successor, so a predecessor that may have bought keeps its own.
    • A pin failure on the fan-out is handled per account, like a credential failure, and keeps the existing multi-account aggregation. I didn't change how the root row's error is classified there, to keep this scoped.
    • The GetPlanAccounts error and the #1537 reattachAccountScope refusal are unchanged, since they aren't part of this PR.
  4. Fixed in 5dc755f. The down migration's header now says it is lossy: single pins are dropped, and re-applying up restores only the multi pins its backfill can prove.
  5. Not done by the author. The author doesn't have label permissions, so a maintainer needs to mirror triaged, priority/p1, severity/high, urgency/this-quarter, impact/many, effort/m and type/bug. On size, I'd ask for an exception rather than a split. The store method, migration and executor change only make sense together: the migration alone is dead schema, and the executor change alone doesn't compile. Most of the lines are tests and mocks.

Verification at 5dc755f (local, macOS, Go 1.26.9; no real cloud calls):

  • Fail-before: I ran the new tests against 766dc53's internal/purchase/{execution,fan_out_shape}.go.
  • go build ./..., go vet ./... and go vet -tags=integration ./...: clean.
  • golangci-lint config verify && golangci-lint run --timeout=10m ./... (v2.10.1, as in CI): 0 issues.
  • AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./... -count=1: all pass. Without that isolation, a few api, email and server tests fail on this machine's ~/.aws/config; that isn't related to this change.
  • Real Postgres (testcontainers), all PASS:
    • TestMigration_PurchaseExecutionFanOutShapes and TestMigrationsAreTransactional
    • TestPinExecutionFanOutShape, which now also covers the read: empty before a pin, pinned shape after
    • the full go test -tags=integration ./internal/purchase/ suite
  • Still not covered: real-cloud and HTTP Retry end-to-end.

@cristim cristim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent delta review of 5dc755f (previous: 766dc53): draft: blocks ready (CI and CodeRabbit only; no code findings)

OLD_SHA is an ancestor of NEW_SHA (one new commit). Base origin/main is still e003025 and the PR merges cleanly (git merge-tree).

Previous findings

# Finding Status Evidence
1 (blocking) Pin written before pre-submission checks RESOLVED execution.go:110-112 pins only after resolveSingleAccountProvider succeeds; execution.go:297-303 pins in executeForAccount after resolveAccountProvider succeeds. lineageFansOut (fan_out_shape.go:56-80) now only reads via GetExecutionFanOutShape.
2 (nit) Colon guard untested RESOLVED fanOutPinKey (fan_out_shape.go:38-47); TestColonLineageKeysAreNeverPinned asserts neither Get nor Pin is called for root-lineage:acct-A.
3 (nit) Refusals not beforeSubmission RESOLVED Pinned-multi/no-accounts, unknown shape, failed pin read, single-path pin failure and pin mismatch are all beforeSubmission (fan_out_shape.go:65,76,80,99,103). The fan-out pin failure is handled per account like a credential failure.
4 (nit) Down migration lossy RESOLVED Header added to 000123...down.sql.
5 Labels, size Labels now present and match #97. Size is 789 added lines against the 400 guideline; the author's exception request (migration, store method and executor only work together, most lines are tests and mocks) is reasonable and non-blocking.

Proof for finding 1 (exact refuse-then-retry scenario)

TestRetryAfterPreSubmissionRefusalFollowsReconfiguredPlan (no plan accounts, recs span acct-A and acct-B; attempt 1 refused; both accounts attached; retry):

  • At NEW_SHA: pass. Attempt 1 leaves nothing pinned; the retry fans out and buys under lineage-97:acct-A and lineage-97:acct-B.
  • With OLD_SHA's execution.go and fan_out_shape.go restored and the NEW tests: FAIL, "Should be empty, but was single", and the retry is routed to the single-account path. TestConcurrentAttemptPinnedOtherShapeBuysNothing also fails there.
  • Mutation: moving the single-path pin back above resolveSingleAccountProvider fails TestRetryAfterPreSubmissionRefusalFollowsReconfiguredPlan.

Double-buy re-attack on lazy pinning

Can two concurrent attempts of one lineage choose different shapes? Both can read "nothing pinned" and pick different live shapes, but the shape only matters at submission, and the pin is a first-writer-wins upsert (store_postgres_fan_out_shape.go:33-40, ON CONFLICT DO UPDATE SET shape = existing, serialized by the row lock). The loser gets the winner's shape back and pinLineageShape (fan_out_shape.go:99-104) refuses before any provider call. Every provider-calling path passes through a pin first: single path at execution.go:110, each fan-out account at execution.go:297 (always on the root key; executeForAccount receives the root baseExec). Mutation: disabling the mismatch refusal fails TestConcurrentAttemptPinnedOtherShapeBuysNothing. Mixed outcomes in one fan-out (account A pinned and bought, account B failed credentials) leave multi pinned, which is correct since A's tokens exist. No new finding.

Residual (non-blocking, not fixable without per-rec pinning): if processPurchaseRecommendations fails before its first provider call after the pin, the lineage stays pinned. That matches "can submit" and is conservative (it refuses rather than double-buys).

Migration number

000123 is free: origin/main latest is 000122. Of #904-#909 only #904 carries a migration, and it is 000124 (000124_ramp_ledger_retention); #905-#909 add none. No clash now; whichever of #896 and #904 lands second needs no renumber, but #904 depends on 123 being used or skipped, so keep that order in mind.

What I attacked

  • go build ./...; go test of internal/purchase, internal/config, internal/mocks, internal/analytics (AWS config isolated): 1701 passed.
  • Integration (testcontainers Postgres, auto-removed): TestPinExecutionFanOutShape and migration tests in internal/config (5 passed), FanOut and TestMigrationsAreTransactional in the migrations package (2 passed).
  • Mutants (all reverted): colon guard disabled fails both scopeless subtests; mismatch refusal disabled fails the concurrency test; early single-path pin fails the retry test.
  • Not run: full go test ./..., real-cloud or HTTP Retry end-to-end.

Gate status

  • Independent review naming the SHA: this comment.
  • Actionable findings resolved: yes; the one blocking item is fixed.
  • CI on the SHA: not green. pre-commit (run 38823078911) and CI - Build & Test (run 38823079188) are action_required (fork approval pending). CodeRabbit skipped the draft; no CR review exists for this SHA.
  • Local real-path evidence: present on the PR (author, macOS) and reproduced above.
  • Verdict recorded on the PR: yes.
  • Labels: mirror #97 (triaged, priority/p1, severity/high, urgency/this-quarter, impact/many, effort/m, type/bug).

Blocks ready

  • Approve and get green runs 38823078911 and 38823079188 on this SHA.
  • Take PR out of draft and run the CodeRabbit loop on it (or record that independent review plus local verification cover it per the gate).
  • Any new commit restarts the gate.

Extract the single-account pre-submission steps (row-account check,
provider resolution, fan-out shape pin) into
prepareSingleAccountSubmission. No behavior change.

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

IAmJSD commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

CI fix: the gocyclo pre-commit hook flagged (*Manager).executeSingleAccount at 11 on 5dc755f. New head dec4b31 moves the single-account pre-submission steps (row-account check, provider resolution, fan-out shape pin) into prepareSingleAccountSubmission. Behavior is unchanged.

Verification at dec4b31:

  • gocyclo -over 10 $(git ls-files "*.go" | grep -v _test.go | grep -v vendor/) prints nothing.
  • go build ./..., go vet ./... and go vet -tags=integration ./... are clean.
  • golangci-lint v2.10.1: 0 issues.
  • go test ./internal/purchase/... passes with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, including all seven fix(purchase): a plan crossing zero-to-N cloud accounts between attempts silently changes every idempotency token #97 tests.
  • pre-commit isn't installed here, so I ran the cheap hooks by hand on the changed files: gofmt, go mod tidy, the migration-number duplicate check, trailing whitespace and EOF newlines. All are clean. gosec and git-secrets aren't installed locally, so I didn't run them.

@cristim cristim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent delta review of dec4b31 (previous: 5dc755f): no blocking findings

Delta. OLD_SHA is an ancestor of NEW_SHA. One new commit, dec4b31 "refactor(purchase): keep executeSingleAccount under the gocyclo budget". It is not a rebase, so there is no range-diff. The diff touches only internal/purchase/execution.go (+21/-8). It moves requireRecsMatchRowAccount, resolveSingleAccountProvider and pinLineageShape(Single) into a new prepareSingleAccountSubmission (execution.go:160-172), called at execution.go:103.

Double-buy and skipped-buy analysis.

  • Order is unchanged: recs check, provider resolve, pin, then submit. The pin still runs after the last pre-submission check and before any provider call, so a lineage cannot submit under a second shape.
  • Every error stays beforeSubmission-wrapped. pinLineageShape wraps its own errors (fan_out_shape.go:97, :100), the helper returns them as-is, and the caller returns err as-is. No wrap is lost or doubled, so a pre-submission failure is still retryable (no skipped buy) and cannot be mistaken for a post-submission failure (no double buy).
  • No shadowing, and the returned values are identical. The multi-account pin at execution.go:312 is untouched. No behavior change.

Previous findings.

Finding Status Evidence
Pre-submission pin finding RESOLVED (unchanged) same logic, now in execution.go:160-172
Code findings none open n/a

Migration. 000123_purchase_execution_fan_out_shapes (up/down/test) is free: origin/main (d6ffd5d) ends at 000122. #904 uses 000124_ramp_ledger_retention. No other open PR touches 000123 or higher.

Conflicts. git merge-tree --write-tree origin/main dec4b318 merges cleanly. The PR base is still e003025, and main has since gained #892 (d683647) and #899 (d6ffd5d), which do not touch this code. GitHub reports MERGEABLE.

Local verification (NEW_SHA, detached worktree, macOS).

  • go build ./...: ok
  • go vet ./internal/purchase: clean
  • go test ./internal/purchase/...: 695 passed
  • gocyclo -over 14 internal/purchase/execution.go: no output, so under the budget of 15
  • golangci-lint run ./internal/purchase/... panicked inside the tool; not used as evidence.
  • Mutation probes skipped: pure extraction, no new logic.

Gate status.

  • Independent adversarial review of the full diff, naming the SHA: this comment.
  • Findings: none blocking.
  • CI on dec4b31: pre-commit (run 38827459155) and CI - Build & Test (run 38827459144) are queued. No action_required runs. CI must be green before ready.
  • Local verification: recorded above.
  • CodeRabbit: skipped (draft and quota-limited); covered by this independent review plus local verification.

Blocks ready: CI green on dec4b31 (pending, not a code finding).

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member

Gate record for dec4b31 (origin/main CLAUDE.md "Review gate"):

  • Independent reviews: full review at 766dc53, delta at 5dc755f (the pre-submission pin finding is resolved), and delta at dec4b31 (a pure gocyclo extraction with the order of checks unchanged). No open findings. The size is over the 400-line guideline, which is non-blocking.
  • CI on dec4b31: CI Success pass, Run pre-commit hooks pass.
  • Local verification (macOS):
    • Refuse-then-retry: TestRetryAfterPreSubmissionRefusalFollowsReconfiguredPlan fails on the 766dc53 code and passes here.
    • Concurrent shape race: TestConcurrentAttemptPinnedOtherShapeBuysNothing refuses the loser before any provider call.
    • Store and migration tests pass on testcontainers Postgres.
    • purchase package: 695 tests pass.
    • No real-account purchase.
  • Migration 000123 is free on main. fix(storage): retain unfinished ramp execution history #904 (000124) merges after this PR so the versions stay ordered.
  • CR skipped: draft and quota-limited; covered by the independent reviews plus local verification.
  • Labels mirrored from fix(purchase): a plan crossing zero-to-N cloud accounts between attempts silently changes every idempotency token #97.

Marking ready (owner-authorized) and merging at dec4b31 with --match-head-commit.

@cristim
cristim marked this pull request as ready for review October 16, 2026 00:38
@cristim
cristim merged commit b4babd0 into LeanerCloud:main Oct 16, 2026
25 checks passed
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-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(purchase): a plan crossing zero-to-N cloud accounts between attempts silently changes every idempotency token

2 participants