Repository navigation
fix(purchase): pin each execution lineage's fan-out shape - #896
Conversation
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>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 13
✨ Finishing Touches
Comment |
cristim
left a comment
There was a problem hiding this comment.
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
-
Actionable.
internal/purchase/fan_out_shape.go:41-45(called frominternal/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 pinssingle, thenresolveSingleAccountProviderrefuses 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 readssingle, 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 stampedno_submissionreplace 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. -
Actionable (test gap).
internal/purchase/fan_out_shape.go:38. Mutation probe: changing the guard tokey == ""(dropping thestrings.Contains(key, ":")skip) leavesgo test ./internal/purchase/green. Nothing asserts that a per-account (K:A) or scopeless colon key is never sent toPinExecutionFanOutShape. 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 scopelessK:acct-Arow and asserts no pin call is made. -
Nit.
fan_out_shape.go:51,53,59,62. The multi-no-accounts refusal and the pin-failure refusal are plain errors, notbeforeSubmission(...), so the rows are not stampedno_submission. This matches the existingGetPlanAccountserror andreattachAccountScoperefusal, 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. -
Nit. Down migration
000123...down.sqldrops the table, so pinnedsinglerows are lost and a re-apply of up only restoresmulti(backfill by design). Acceptable, but worth a line in the migration header. -
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 testofinternal/purchase,internal/config,internal/mocks,internal/analyticsat the SHA: pass.go vetof purchase and config: clean.- Fail-before: restored
internal/purchase/execution.gofromorigin/mainwith the PR's tests:TestRetryAfterPlanGainsFirstAccountReusesSingleAccountToken,TestRetryAfterPlanLosesLastAccountBuysNothingandTestFanOutShapePinFailureBuysNothingfail;TestUnchangedPlanKeepsItsTokensAcrossRetriespasses (back-compat control). All pass at the head. - Mutation probes in
fan_out_shape.go: disabling the pinned-singlebranch fails the zero-to-N test; returning!liveon a match fails three tests; dropping the colon guard survives (finding 2); retargeting the pinned-multicase to a dead value survives only because thedefaultbranch also errors (equivalent mutant). - Store: changing
DO UPDATE SET shape = <existing>toEXCLUDED.shapefailsTestPinExecutionFanOutShape(first-writer-wins and concurrent subtests), so the no-overwrite semantics are covered. - Real Postgres (throwaway container):
TestMigration_PurchaseExecutionFanOutShapes,TestMigrationsAreTransactionalandTestPinExecutionFanOutShapepass at the SHA (backfill, idempotent up, CHECK constraint, down). - Read every other
GetPlanAccountscaller (plan_step_attach.go:147,api/scoping.go:262,api/handler_plans.go:532): none derive purchase tokens, soexecutePurchaseis the only path-selection point. Retry key propagation (handler_purchases.go:2192-2204) matches the pin key. - Not run:
internal/purchaseintegration-tag suite and the fullgo 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 & Testandpre-commitareaction_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_submissionretries 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.
|
Clarification of the review above (766dc53), after maintainer calibration. Only these block ready:
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>
|
Reply to the independent review of 766dc53. The new head is 5dc755f (one new commit, no force-push).
Verification at 5dc755f (local, macOS, Go 1.26.9; no real cloud calls):
|
cristim
left a comment
There was a problem hiding this comment.
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-Aandlineage-97:acct-B. - With OLD_SHA's
execution.goandfan_out_shape.gorestored and the NEW tests: FAIL, "Should be empty, but was single", and the retry is routed to the single-account path.TestConcurrentAttemptPinnedOtherShapeBuysNothingalso fails there. - Mutation: moving the single-path pin back above
resolveSingleAccountProviderfailsTestRetryAfterPreSubmissionRefusalFollowsReconfiguredPlan.
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 testofinternal/purchase,internal/config,internal/mocks,internal/analytics(AWS config isolated): 1701 passed.- Integration (testcontainers Postgres, auto-removed):
TestPinExecutionFanOutShapeand migration tests ininternal/config(5 passed),FanOutandTestMigrationsAreTransactionalin 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) andCI - Build & Test(run 38823079188) areaction_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>
|
CI fix: the Verification at dec4b31:
|
cristim
left a comment
There was a problem hiding this comment.
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.
pinLineageShapewraps its own errors (fan_out_shape.go:97, :100), the helper returns them as-is, and the caller returnserras-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 ./...: okgo vet ./internal/purchase: cleango test ./internal/purchase/...: 695 passedgocyclo -over 14 internal/purchase/execution.go: no output, so under the budget of 15golangci-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).
|
Gate record for dec4b31 (origin/main CLAUDE.md "Review gate"):
Marking ready (owner-authorized) and merging at dec4b31 with --match-head-commit. |
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 fromK:<accountID>.executePurchasepicked the path from the plan's liveplan_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 newpurchase_execution_fan_out_shapestable 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:singleDerive(K, i)and the provider dedupesmultiKeys with a colon (scopeless per-account rows, #1537) and empty keys skip the pin, and
reattachAccountScopestill 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)andDerive("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
multifor every root lineage that already has a per-account (K:A) row, because only the fan-out writes one. It deliberately does not infersinglefrom 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.
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 unmodifiedorigin/main+ test file only):and pass after it:
Other checks:
go build ./...: OKgo test ./... -count=1: all pass. One test,internal/serverTestWireLadderWriteSide_ExecutionDisabled_RealAWSLadder_RefusesWithoutAWSCall, fails only because of this machine's~/.aws/config(source_profile requires role_arn). It passes withAWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./internal/server/, and it isn't related to this change.go vet ./...andgo vet -tags=integration ./...: OKgolangci-lint config verify && golangci-lint run --timeout=10m ./...with the CI-pinned v2.10.1 (built with Go 1.26.9):0 issues.go test -tags=integration ./internal/database/postgres/migrations/ -run 'TestMigration_PurchaseExecutionFanOutShapes|TestMigrationsAreTransactional': PASS. Covers the backfill (multionly 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/mandtype/bug. A maintainer should mirror these onto this PR, because the author doesn't have label permissions.🤖 Generated with Claude Code