[cristim]

Part of #702 (Redshift re-drive idempotency). This is PR2b, between PR2 (#1807, merged) and PR3. It does not close the issue.

Why

Before buying, the Redshift client in cloud-commitments-go adopts any owned reserved node with the same offering and node count that started at or after RequestedAt minus 5 minutes. It applies no upper bound and doesn't check who bought the node. The platform always sends a token, so this runs on first dispatches too. As a result, a new purchase of a tuple dispatched within about 5 minutes of another lineage's completed buy adopted that node instead of buying its own: an under-buy, with one commitment counted twice. PR2's gate only counted running or still-open lineages, so it didn't close this.

What

• Account precedence: the conflict query now matches a candidate on the execution's account first, then on the rec's account, the same order dispatch uses.
• Adoption window: the gate also counts another lineage's same-tuple row as a conflict when that row is completed, partially_completed or revocation_requested and its updated_at is within 10 minutes of this dispatch's first-dispatch stamp. That is go's 5-minute skew plus a 5-minute clock margin. Both timestamps come from the database clock.
  • The same family is excluded, so a retry never refuses on its own lineage's buys.
  • The refusal happens before any purchase call, with a fixed, allowlisted text that names no other execution: start a new purchase at least 10 minutes after the other one completed.
• Known issues: known_issues/39 marks the entry as mitigated for buys the platform records, and lists what remains:
  • buys made outside the platform;
  • two identical recs in one execution (Redshift: two identical-tuple recommendations in one execution buy one node #1809);
  • a lineage whose purchase record never landed;
  • go-side narrowing (cloud-commitments-go#700);
  • a later save of the other row restarts its window, which refuses longer but never allows a double buy.

Trade-off

A legitimate repeat buy of the same tuple is refused for 10 minutes instead of being silently adopted. Plan steps are spaced in days and ladders fire daily or weekly, so a single plan or ladder never hits this. Two different plans that buy the same tuple on the same tick can.

Until PR3 lifts the Redshift Retry guard, a refused plan row can't be retried, and its ramp step stays held until the account is detached. A refused ladder tranche is resolved as failed and re-bought at its next fire. The owner accepted this trade-off.

Tests

• Integration: an account-precedence subtest, plus ten adoption-window subtests. They cover each finished status, the window boundary, the same-family exclusion, count and account mismatches, the NULL-account wildcard, updated_at used over completed_at, and a completion after an old stamp.
• Gate tests:
  • a recent completed buy is refused before submission, rolls the stamp back, makes no provider call, and stores the text verbatim;
  • a second lineage after the window dispatches;
  • a retry successor in the same family is not refused.

No migration.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 21 minutes.

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: fix/702-redshift-adoption-window
• File: .coderabbit.yaml
• Commit: 404ad8e
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: f2147920-1e7c-4523-9e23-04d285a8602c

Commits

Reviewing files that changed from the base of the PR and between 803090d and 404ad8e.

Files selected for processing: 12

• internal/config/interfaces.go
• internal/config/store_postgres_redshift_lineage.go
• internal/config/store_postgres_redshift_lineage_integration_test.go
• internal/mocks/stores.go
• internal/purchase/armed_redrive_test.go
• internal/purchase/errnotes.go
• internal/purchase/errnotes_test.go
• internal/purchase/redshift_gate_integration_test.go
• internal/purchase/redshift_gate_test.go
• internal/purchase/redshift_lineage.go
• internal/purchase/requested_at_test.go
• known_issues/39_redshift_dedupe_window.md

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

APPROVED 2f0063b by claude-ui-fix-2

Cold review of #702 PR2b (money path, no migration). The code is approved. CI is red on lint, so a fix commit is needed before merge (see the end), and the new head needs a delta review.

Window bounds: no upper-bound gap

The new branch (store_postgres_redshift_lineage.go) matches another family's completed, partially_completed or revocation_requested row when e.updated_at >= stamp - 10m, with no upper bound. Go's Redshift dedupe adopts any node that started at or after RequestedAt - 5m, which is also open-ended. So every later completion is caught, including one that lands after this lineage's stamp.

The proxy can't miss an adoption. A finished row's updated_at (row trigger) is at least its completion, which is at least its node's start. So any node that starts at or after stamp - 5m gives updated_at >= stamp - 5m, which is at least stamp - 10m, and the row conflicts.

Clock source: the database on both sides

stamp is res.At from StampFirstDispatchTx ... RETURNING first_dispatch_at (now()), and updated_at is stamped by the DB trigger. The 5-minute margin over go's skew absorbs the gap between the DB and AWS StartTime clocks. On a later attempt the window is anchored to the original stamp, which matches go anchoring RequestedAt to the lineage stamp.

It can't let an adoption through, within its stated scope

• Finished rows of another family conflict. Same-family finished rows don't, and that's correct: adopting the lineage's own node is the intended dedupe.
• redshiftFinishedStatus refuses with beforeSubmission before any conflict-text path, so nothing is submitted.
• Known and stated gap: failed rows with an unknown outcome aren't in the set until PR3b, as the owner's Q5 A decision accepts.

Tenant and account scope

• The candidate side is now COALESCE($4 execution account, tuple account), which is the account the client actually buys in (FU1). The row side stays the row's account, else the rec's.
• NULL on either side still matches any account, so it fails closed.
• The refusal text is fixed and allowlisted in errnotes.go, and never names the other execution, whose id and status go only to the log.

Refusal text

"…start a new purchase at least 10 minutes after the other one completed". It's fixed and matches the 10-minute window, per the owner's Q7 A.

Runtime (scratch worktree at this SHA, heavy-review-2, GOMAXPROCS=4 -p 2)

• -tags integration ./internal/purchase passes (41s), and ./internal/config passes (84s).
• Probe (drop the 10-minute margin, updated_at >= stamp): TestRedshiftDispatchGate_RecentCompletedBuyRefusedBeforeSubmission fails, and so does a subtest of TestFindOpenRedshiftLineageConflicts_AdoptionWindow. The file was restored and the worktree removed.

CI: must fix before merge

Lint Code fails, and with it CI Success. internal/purchase/redshift_gate_integration_test.go:186:90: (*redshiftGateFixture).finishedRow - result 0 (*config.PurchaseExecution) is never used (unparam). Drop the return value, or use it. That's a new head, which needs a delta review.

[cristim]

CHANGES REQUESTED 2f0063b by claude-qa-ui-4

Blocking:

1. Nothing pins the 10-minute window. Setting redshiftAdoptionWindow (internal/purchase/redshift_lineage.go:54) to 4 minutes, below go's 5-minute redshiftClockSkew (dedupe.go at the pinned 790f369f), which reopens the adoption, passes unit, purchase integration and config integration. redshift_gate_test.go:94 compares the constant with itself, redshift_gate_integration_test.go:221 seeds redshiftAdoptionWindow + 1m (it moves with the constant), and the config tests use their own lineageTestWindow. Please add a gate integration test that seeds the other family's finished row about 6-9 minutes before the stamp (a literal, not derived from the constant) and expects a refusal, or a test asserting the window is at least go's skew plus the margin.

2. CI "Lint Code" fails at this head on the same finding: internal/purchase/redshift_gate_integration_test.go:186 (*redshiftGateFixture).finishedRow result 0 is never used (unparam). The other report (prealloc in store_postgres_history_page_integration_test.go:141) is in an untouched file.

Non-blocking:

• failed rows with no_submission=false (the reaper failing an in-flight buy, or ErrOutcomeUnknown) are not refused and can hold a real node; go adopting that node still keeps the accounting consistent (one node, one completed row). It is deferred to PR3b; a line in known_issues/39 for this window would record it.
• When a finished conflict and a running conflict coexist, the finished-row text wins (redshift_lineage.go:131-136) while the running one may still block after 10 minutes.

Checked and holding:

• Window (store_postgres_redshift_lineage.go:98-100): updated_at >= stamp - window, with no upper bound. The stamp is the DB's RETURNING first_dispatch_at (store_postgres_first_dispatch.go:33-40), and updated_at is DB NOW() on insert and via the BEFORE UPDATE trigger (migrations 000001:20, :237-260), so later updates only extend the refusal.
• Status set {completed, partially_completed, revocation_requested} matches redshiftFinishedStatus; running is caught at any age by the open branch.
• Candidate account is now COALESCE($4, t.cloud_account_id) (:108-109), matching dispatch (execution.go:460-466).
• The refusal text is fixed, allowlisted (errnotes.go:37) and a before-submission error; the stamp rolls back.
• A refused automatic row ends failed with no_submission, which is not a settled ramp status, so the step holds (Q5 A).
• Check, lock and stamp are in one tx with no pool reads.

Runs (GOMAXPROCS=4, -p 2): build and vet pass; unit purchase/config pass; integration purchase/config pass. Probes: 9 of 10 killed (Go refusal dropped, SQL finished branch dropped, partially_completed or revocation_requested dropped from SQL, revocation_requested dropped from Go, COALESCE reverted, app time in Go or SQL, an upper bound added). The 4-minute window survived (finding 1).

[cristim]

APPROVED 404ad8e by claude-ui-fix-2

Delta from my approval at 2f0063b8. There are no production changes: git diff 2f0063b8 404ad8eb09f6ca2ec8f8aa53b67114cf2e4afa72 touches only two test files and a known-issues doc.

• Lint fixed. finishedRow (redshift_gate_integration_test.go) no longer returns its unused *config.PurchaseExecution, which was the unparam CI failure in my review.
• The window is pinned by literal. The gate test now asserts run.window == 10*time.Minute rather than the constant itself, and the new TestRedshiftAdoptionWindow_CoversGoSkewPlusMargin asserts redshiftAdoptionWindow >= 5m + 5m. A constant shrunk below go's skew plus margin now fails a test, which the author's probe confirms (4m fails both). The PR's own test file now records go's 5-minute skew and the extra 5-minute margin I relied on.
• Docs. known_issues/39 now records the gap I noted (failed rows without no-submission proof aren't covered until PR3), and its wording matches the code.

CI at this SHA was still running when I posted (4 pass, 32 pending). The merge gate needs it green, including Lint Code.

[cristim]

APPROVED 404ad8e by claude-qa-ui-4

Delta from my CHANGES REQUESTED at 2f0063b; test and docs only, no production code changed.

• Item 1 fixed: internal/purchase/redshift_gate_test.go:94 now asserts the literal 10*time.Minute, and the new TestRedshiftAdoptionWindow_CoversGoSkewPlusMargin (:230-235) requires the window to be at least 5m (go's skew) + 5m. Setting redshiftAdoptionWindow to 4 minutes now fails TestRedshiftDispatchGate_RecentFinishedBuyRefusedWithFixedText and TestRedshiftAdoptionWindow_CoversGoSkewPlusMargin.
• Item 2 fixed: finishedRow (redshift_gate_integration_test.go:186) no longer returns the unused row. golangci-lint with the integration build tag reports 0 issues on ./internal/purchase/.
• known_issues/39_redshift_dedupe_window.md records the failed rows without no-submission proof inside the window.
• At this head (GOMAXPROCS=4): go test ./internal/purchase/ -run Redshift passes. My earlier full unit and integration runs at 2f0063b cover the unchanged production code.