[cristim]

Part of #702. This is PR2 of 4 for Redshift re-drive idempotency, and it does not close the issue. PR1 (#1762, merged) persisted purchase_executions.first_dispatch_at and passes it to the library as RequestedAt.

Why

The library dedupes a Redshift reserved-node buy with DescribeReservedNodes. It looks for a node that started at or after RequestedAt minus 5 minutes, with no upper bound. If a second lineage buys the same tuple while an earlier one can still be re-driven, that re-drive could adopt the later node, or refuse as ambiguous. Before Redshift re-drive can be allowed (PR3), a dispatch must refuse when another open lineage could still buy the same tuple.

What

• FindOpenRedshiftLineageConflictsTx (internal/config/store_postgres_redshift_lineage.go). It finds other executions that could still buy the same tuple (region, node type, term, payment, count). On the account side it uses the row's account, else the rec's account, and a missing account on either side counts as a match (fails closed). A conflicting execution is one of:

  • any stamped running row other than self, including a parallel run of the same lineage;
  • an approved, scheduled, pending, notified or paused row of another lineage stamped within 24h, judged on the DB clock.

• The dispatch gate (internal/purchase/redshift_lineage.go) runs before any provider call for an execution with a Redshift rec, all in one transaction:

  1. force READ COMMITTED with a lock timeout;
  2. take a pg_advisory_xact_lock per tuple, in sorted order;
  3. stamp the first dispatch;
  4. check for conflicts.

  A conflict rolls the stamp back and refuses before submission. A stamp outside the 24h horizon, or a missing stamp on a later attempt, is a plain error, because an earlier attempt may have bought.

• Refusal texts are fixed and allowlisted for History. The conflict text does not name the other execution; its id and status go to the server log only.

• Unchanged: Redshift re-drive and Retry are still refused. failed rows join the conflict set together with the guard lift (PR3b).

Review

• The plan had two adversarial passes before any code.
• Each commit had two non-author passes, with mutation probes and the integration suites for the touched packages.
• Cold PR reviews are still to do.

Tests

• Unit and integration tests cover the tuple and account matrix, and the lock, stamp and find order and isolation.
• Race and rollback integration tests check that two first dispatches of one tuple buy exactly once, including a root execution and its per-account successor.
• Mutation probes each make a test fail:
  • drop the tuple lock;
  • ignore a conflict;
  • put an execution id in the conflict text;
  • exclude every same-family row, even a running one.

No migration. A follow-up refactor will make the tuple lock and the plan lock share one advisory-lock key helper.

🤖 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 33 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-lineage-conflict
• File: .coderabbit.yaml
• Commit: b2ccdef
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: c6c0f432-c3c9-4193-af5c-90fef9ba38c6

Commits

Reviewing files that changed from the base of the PR and between 9f98f15 and b2ccdef.

Files selected for processing: 13

• internal/config/interfaces.go
• internal/config/store_postgres_redshift_lineage.go
• internal/config/store_postgres_redshift_lineage_integration_test.go
• internal/config/types.go
• internal/mocks/stores.go
• internal/purchase/armed_redrive_test.go
• internal/purchase/errnotes.go
• internal/purchase/errnotes_test.go
• internal/purchase/ramp_step_progress_integration_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

• 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 b2ccdef by claude-ui-fix-2

Cold review of #702 PR2 (+983/-15, no migration), judged as fail-closed: the gate may refuse more than it needs to, but it must never let a duplicate through.

Where the gate sits

executeAndFinalize (internal/purchase/manager.go:326-334) runs:

1. preExecutionRefusal;
2. stampRedshiftDispatch;
3. executePurchase.

That's after the CAS claim to running and before any provider call.

StampFirstDispatchTx (store_postgres_first_dispatch.go:31-41):

• updates only WHERE status = 'running', so it's bound to the claim;
• keeps an existing stamp (COALESCE);
• mints one only on attempt 0, or when the parent provably submitted nothing.

The gate (redshift_lineage.go) does the following in one transaction:

• LockRedshiftTuplesTx: READ COMMITTED, lock_timeout for the waits only, then reset, with pg_advisory_xact_lock(hashtextextended(key)) in sorted key order, so there's no deadlock.
• The stamp, then the conflict lookup.
• On a conflict it returns beforeSubmission, the transaction rolls the stamp back, and the locks are released at the transaction's end.
• A lock-timeout or other error at attempt 0 is also beforeSubmission.
• A missing or past-horizon stamp on a later attempt is a plain error, never "nothing was sent".

Concurrent dispatches of the same tuple

Take A and B, two lineages:

1. Both rows are claimed running.
2. A takes the tuple lock, stamps itself, finds B unstamped (first_dispatch_at IS NOT NULL is required), commits, and buys.
3. B gets the lock and, under READ COMMITTED, sees A as running and stamped, which is a conflict, so B is refused before submitting.

The same holds while A sits in approved, scheduled, pending, notified or paused with a stamp inside 24h.

Fan-out children don't gate. They inherit the root's stamp, and as running stamped rows they block other lineages. A same-family re-drive while children run is refused, which is fail-closed. TestRedshiftDispatchGate_TwoLineagesRaceOnlyOneBuys and …RootAndAccountSuccessorsOfOneFamilyDoNotBothBuy pin this.

Lock keys, normalization and case

LockKey and the SQL tuple match use the raw rec strings. A spelling difference can't let a duplicate through:

• Payment is canonicalized before storage: lowercase plus NormalizePaymentOption (api/validation.go:656-685).
• Node type must equal AWS's offering spelling exactly to buy anything (providers/aws/services/redshift/client.go: *offering.NodeType != rec.ResourceType).
• Region selects the per-region client.
• Term and count are ints.

So two spellings of one tuple can't both purchase. The service match agrees with Go too: isRedshiftRec goes through mapServiceSlug, whose only Redshift keys are redshift and data-warehouse, equal to the common constants, and the SQL matches IN ('redshift','data-warehouse') with provider '' or aws.

Tenant scope

• The lock key leaves out the account, which only serializes cross-tenant dispatches of the same tuple.
• The conflict query treats a NULL account on either side as matching any account (fail-closed). A non-UUID rec account counts as NULL.
• Two different non-NULL accounts don't conflict, which is correct: Go's dedupe runs DescribeReservedNodes inside one account, so it can't adopt across accounts.
• The refusal text is fixed and allowlisted, and the other execution's id and status go only to the log.

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

• -tags integration ./internal/purchase passes (44s), and ./internal/config passes (90s).
• Probe A (the author's, re-run): if len(conflicts) > 0 changed to never fire makes four gate tests fail, including TwoLineagesRaceOnlyOneBuys and ConflictRefusesBeforeSubmission.
• Probe B (new): removing the candidate-side NULL wildcard (OR COALESCE(t.cloud_account_id, $4::uuid) IS NULL) makes TestFindOpenRedshiftLineageConflicts/account_matrix fail.
• Both files were restored and the worktree removed.

CI at this SHA: 40 pass, 1 skipped, 0 failing.

[cristim]

APPROVED b2ccdef by claude-qa-ui-4

Cold review, judged fail-closed (no duplicate may pass; over-refusal is acceptable).

• Order: the CAS claim (internal/purchase/manager.go:218-240) commits running, then executeAndFinalize runs preExecutionRefusal, then stampRedshiftDispatch, then executePurchase (manager.go:327-334). Lock, stamp and conflict find run in one WithTx under READ COMMITTED (internal/config/store_postgres_redshift_lineage.go:40-60, :79-131). Two lineages race on one tuple: the second waits on the xact lock, then sees the first's stamped running row and is refused. A claimed-but-unstamped row is excluded (first_dispatch_at IS NOT NULL) and runs its own gate later, so either order is safe. Self is excluded (<> $1).
• Every dispatch path goes through executeAndFinalize: cron sweep (manager.go:1014 → :240), SQS (messages.go:135), approve, direct and Run now (approvals.go:482), the delayed fire (scheduled_fire.go:163), the stranded re-drive (manager.go:701; Redshift is refused earlier there), retry successors via those, and the fan-out under the root's gate. The only provider purchase call is execution.go:1230.
• Lock key: redshift-tuple:Region|NodeType|Term|Payment|Count (store_postgres_redshift_lineage.go:28), hashed with hashtextextended into pg_advisory_xact_lock, in sorted order. The account is left out, and a collision only serializes more; refusal comes from the SQL match. The 30s lock_timeout is reset after the waits. The locks are xact-scoped and released on commit or rollback (the WithTx deferred rollback covers panics). No pooled reads run inside the tx.
• The refusal texts are fixed, name no ids, and are allowlisted in errnotes.go.

Runs (testcontainers, GOMAXPROCS=4, -p 2):

• Build and vet pass.
• Unit purchase, config and mocks pass.
• Integration ./internal/config/ and ./internal/purchase/ pass.
• golangci-lint: one prealloc in store_postgres_history_page_integration_test.go:141, an untouched file.
• Probes, each killed:
  • drop the advisory lock (fails TwoLineagesRaceOnlyOneBuys and TestLockRedshiftTuplesTx);
  • ignore conflicts (fails ConflictRefusesBeforeSubmission, ConflictRollsBackStamp and both race tests);
  • drop the candidate NULL-account wildcard;
  • running rows no longer conflicting.

Non-blocking:

• store_postgres_redshift_lineage.go:101-103: the candidate side uses COALESCE(t.cloud_account_id, $4) (rec account first), while the row side and dispatch use the row account first (execution.go:460-466). Today a mismatch is unreachable (account_scope.go:69-84; direct rows have a nil account), but the gate runs before that check (manager.go:329 vs execution.go:122). COALESCE($4, t.cloud_account_id) would follow the dispatch account on both sides.
• completed and partially_completed rows are not in the conflict set (SQL :89-94). With go's dedupe window (RequestedAt minus 5 min), a new lineage dispatched within about 5 min of another lineage's completed buy can adopt that node: an under-buy, with the commitment attributed twice. Worth a follow-up with PR3.
• term and count compare as text (r->>'term' = t.term::text). That is safe for Go-marshalled ints; a numeric compare would be sturdier.