[cristim]

Part of #702. This is PR1 of 4 for the Redshift re-drive idempotency work. It does not close the issue.

Why

The go library dedupes a Redshift reserved-node buy with DescribeReservedNodes. It looks for a node that started no earlier than RequestedAt minus 5 minutes (cloud-commitments-go#509). Since #1138, the platform passes the current time at each dispatch. A re-drive of the same lineage would therefore search a later window and could miss a node the first attempt bought. Redshift re-drive is refused today for exactly this reason.

This PR persists the time of the first dispatch and passes that time instead, so every attempt of a lineage gets the same window start. Re-drive and retry stay refused here. PR2 adds the cross-lineage conflict check, PR3a the Retry gates, and PR3b lifts awsPurchaseHasNoDuplicateGuard for Redshift.

What

• feat(config): migration 000154 adds a nullable purchase_executions.first_dispatch_at.
  • PurchaseExecution.FirstDispatchAt is server-only (json:"-").
  • It is written once at INSERT and left out of the upsert's DO UPDATE SET.
  • Every full-row projection reads it.
• feat(config): StampFirstDispatchTx stamps the claimed running row inside the caller's transaction.
  • It takes the DB's now(), and COALESCE keeps an existing stamp.
  • It mints a stamp only at attempt 0, or when the predecessor is no_submission at attempt 0.
  • It also reports whether the stamp is within a horizon, judged on the DB clock.
• fix(purchase): executeAndFinalize stamps an execution that has a selected Redshift rec, after the pre-execution refusals and before the provider call. The stamp becomes PurchaseOptions.RequestedAt.
  • Fan-out per-account rows inherit the root stamp and are never stamped themselves.
  • A stamp error at attempt 0 fails before submission.
  • A missing stamp on a later attempt is a plain error, so the lineage is never re-stamped.
  • A Redshift rec that reaches the provider without a stamp is refused.

Review

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

Deploy order

• Prod (auto_migrate=false): apply migration 000154_purchase_executions_first_dispatch_at BEFORE deploying this code. Never down-migrate it while this code is live. Every full-row purchase_executions read and insert names the new column. Code running without it fails loud with 42703 (no execution loads, nothing is bought) until the migration runs, the same as the created_by_system precedent. Old code against the migrated schema is safe, since the column is additive and nullable.
• Dev: auto-migrates on merge. Migrations run at cold start before the stores initialise (internal/server/app.go), so no manual step is needed.

Migration number

000154, the next free number on main at merge time, under the merge-time numbering policy. The rename is the final commit and is rename-only: the migration SQL is unchanged, and its test and one comment follow the number. Migrations apply in order, so a later PR must take a higher number.

Owner decisions

These are on #702, with fail-closed defaults. None affects this PR's behaviour: the 24h horizon is computed but not yet enforced.

Tests

Run on the rebased head:

• make build, make lint;
• go test ./internal/...;
• go test -tags=integration ./internal/config/... ./internal/database/postgres/migrations/... ./internal/purchase/... (Postgres testcontainers).

The only lint finding is the existing prealloc at internal/config/store_postgres_history_page_integration_test.go:141, which is unchanged from main.

🤖 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 44 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-first-dispatch-stamp
• File: .coderabbit.yaml
• Commit: fe14519
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 02ccce1c-260d-447e-b573-00f5b094ec84

Commits

Reviewing files that changed from the base of the PR and between 4c02609 and fe14519.

Files selected for processing: 20

• internal/config/interfaces.go
• internal/config/store_postgres.go
• internal/config/store_postgres_first_dispatch.go
• internal/config/store_postgres_first_dispatch_at_integration_test.go
• internal/config/store_postgres_first_dispatch_integration_test.go
• internal/config/store_postgres_history_page.go
• internal/config/store_postgres_pgxmock_test.go
• internal/config/store_postgres_ramp_baseline.go
• internal/config/store_postgres_ramp_step.go
• internal/config/types.go
• internal/database/postgres/migrations/000154_purchase_executions_first_dispatch_at.down.sql
• internal/database/postgres/migrations/000154_purchase_executions_first_dispatch_at.up.sql
• internal/database/postgres/migrations/000154_purchase_executions_first_dispatch_at_test.go
• internal/mocks/stores.go
• internal/purchase/armed_redrive_test.go
• internal/purchase/coverage_extra_test.go
• internal/purchase/execution.go
• internal/purchase/manager.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]

@coderabbitai review

[coderabbitai]

Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

APPROVE @ 3a6caba
Cold review, money path (claude-cc-libs)

Three commits on merge-base 4c02609. go is pinned at 61111428a353, and only the Redshift dedupe reads RequestedAt (redshift/dedupe.go:89, :93, :114).

Before 000158 is applied. Every full-row projection and the INSERT now name first_dispatch_at (store_postgres.go:1181, :1422, :1686, scan at :2253). Without the column, every read or insert of purchase_executions fails with 42703. That failure is loud: no execution loads, so nothing is bought, and the stamp can't be skipped silently. In dev, migrations run at cold start before the stores re-initialise (internal/server/app.go:746-777). Prod has auto_migrate=false (the aws, azure and gcp github-prod.tfvars), and the deploy workflows never run the migration (finding 1). Old code against the new schema is safe, because the column is additive and nullable.

The stamp is written once

Path	Result
First dispatch, attempt 0	The claim sets running. The stamp then commits DB now() in its own transaction (store_postgres_first_dispatch.go:31-40), before executePurchase (manager.go:329-334).
Concurrent first dispatch	The CAS claim admits one runner. Row lock plus COALESCE give both the same value.
Crash after the stamp, before buying	The stamp persists, and a same-row re-drive keeps it through COALESCE.
Outcome unknown, then Retry	The successor is refused by armedRedriveRefusal (manager.go:525) before any stamp. Without a copied stamp it gets nil and is refused (redshift_lineage.go:51-58).
Upsert	first_dispatch_at isn't in DO UPDATE SET.
Fan-out	Per-account copies inherit the root stamp; child rows aren't stamped themselves.

RequestedAt reaches the provider. The path is execution.go:645-647, then recOpts, then withDispatchTime (redshift_lineage.go:66-76), then PurchaseCommitment (execution.go:1230). That's the only call site, and all four executeAndFinalize callers reach it. The value is a timestamptz in microseconds, normalised to UTC, and identical on every read. Non-Redshift recs still get time.Now() per rec; in a mixed execution they get the stamp and ignore it.

The down migration (DROP COLUMN IF EXISTS) removes only what up added. Running up, down, up in a scratch test restored the pre-158 schema exactly, kept the rows, and brought the column back as NULL.

Tests.

• go build ./..., vet and go test ./internal/... pass.
• -tags integration with testcontainers passes for config, migrations and purchase.

Fail on main. With main's non-test sources, the purchase and config integration tests don't build (StampResult and FirstDispatchAt undefined), and six config pgxmock tests fail. TestMigration_0158_FirstDispatchAt failed at :38 in a verbose run; its first quiet run reported ok, which I couldn't explain.

Mutation probes (each reverted)

1. Overwriting the stamp on re-drive: 6 TestStampFirstDispatch_* tests fail.
2. Stamping after the provider call: 3 TestExecuteAndFinalize_Redshift* tests fail.
3. Dropping RequestedAt from the opts: 3 tests fail.
4. Adding the column to DO UPDATE: TestSavePurchaseExecution_FirstDispatchAtIsInsertOnly fails.

There's no security surface (json:"-", parameterised SQL) and no duplication (isRedshiftRec reuses mapServiceSlug).

Findings (none blocking)

1. Medium, ops. In prod, run 000158 before the code deploy. If the code ships first, every purchase read fails until the migration runs, the same as the created_by_system precedent. Put it in the release notes.
2. Low. The comment at internal/config/types.go:403-405 says "written once at INSERT, never updated", but StampFirstDispatchTx UPDATEs the column. Suggested wording: insert-only in the upsert, set by the stamp.
3. Low, YAGNI. WithinHorizon (store_postgres_first_dispatch.go:41) is computed but unused (redshift_lineage.go:46-62). Its caller is PR2; drop it if PR2 slips.
4. Info, for PR3a. Creating a Retry successor (internal/api/handler_purchases.go:2775-2791) doesn't copy FirstDispatchAt, so the "kept once set" successor path exists only in tests today.

Not merging.

[cristim]

Thanks @cc-libs. Responses to the non-blocking findings:

1. Ops: run migration 000158 before the code deploy in prod. Agreed; this is a release step. In prod (auto_migrate=false), apply 000158_purchase_executions_first_dispatch_at before deploying code from this PR. Every full-row purchase_executions read and insert names the column, so code without the migration fails loud with 42703 and buys nothing. Code without the migration does not buy silently.
2. The types.go:403-405 comment is inaccurate. The fix lands in PR2 of fix(purchase): Redshift reserved-node purchases without tag-based idempotency #702 (stacked on this one), not as an amend to this reviewed head. New wording: insert-only in the upsert, set once by the dispatch stamp.
3. WithinHorizon is unused. PR2's dispatch gate uses it for the 24h re-drive horizon.
4. Retry successors don't copy FirstDispatchAt. Correct, and planned for PR3a. Until then Redshift Retry stays refused before any stamp, as you traced.

[cristim]

@coderabbitai review

[coderabbitai]

Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Money-path cold review: APPROVE at 3a6caba, with one rollout requirement for the PR body or release note. Base main 4c02609.

The four requested checks

1. Safe before 000158 is applied: conditionally. The new code is not schema-tolerant. first_dispatch_at is in the INSERT and in every full-row SELECT of purchase_executions (store_postgres.go, store_postgres_history_page.go, store_postgres_ramp_baseline.go, store_postgres_ramp_step.go). Review-only integration probe: I dropped the column from a migrated test DB, then called the store:

   • GetExecutionByID and GetPendingExecutions → column "first_dispatch_at" does not exist (SQLSTATE 42703);
   • SavePurchaseExecution → the same, on the INSERT.

   So without 000158 the whole purchases area fails (history, approvals, scheduling, execution), not only Redshift stamping. specs/migration-resilience.md says the app keeps serving "every request that doesn't need the missing schema", and with this change every purchase request needs it.

   • dev (github-dev.tfvars: database_auto_migrate = true) and staging auto-migrate at startup, before the stores are initialised (internal/server/app.go:740-770). 000158 is a nullable ADD COLUMN IF NOT EXISTS, so in practice it applies before new code serves.
   • prod (github-prod.tfvars: database_auto_migrate = false) migrates by hand through database-migration.yml.

   Required: state in the PR body or release note that 000158 must run on prod before this code deploys. Also, a down-migration of 000158 must not be applied while this code is live; roll the code back first.

2. Stamp written once, never overwritten on redrive: holds.

   • StampFirstDispatchTx uses COALESCE(first_dispatch_at, …). A redrive of the same execution_id returns the original stamp.
   • The upsert's DO UPDATE SET leaves first_dispatch_at out.
   • A new stamp is minted only at attempt 0, or when the predecessor (p.retry_execution_id = this.execution_id, the direction of migration 000042) is no_submission at attempt 0.

3. RequestedAt passed to the provider: holds.

   • processPurchaseRecommendations sets opts.RequestedAt = exec.FirstDispatchAt (execution.go:645-647), and fan-out rows copy the root exec.
   • withDispatchTime refuses a Redshift rec without a stamp instead of defaulting to now.
   • Only the Redshift dedupe reads RequestedAt (cloud-commitments-go providers/aws/services/redshift/dedupe.go:89-119), so passing the stamp for the other recs in a mixed execution changes nothing.

4. Down migration reversible: yes. DROP COLUMN IF EXISTS first_dispatch_at, covered by TestMigration_0158_FirstDispatchAt. The stamps are lost on the way down, which is acceptable for a server-only column.

Mutation probes (all 8 killed)

• Stamp kept, not re-minted: COALESCE removed, so the column is re-stamped on every dispatch. Four TestStampFirstDispatch_* tests fail.
• Predecessor clause: removing the no_submission predecessor clause fails …RetryOfNoSubmissionAttempt0Mints. Minting at any attempt fails …RetryWithoutStampIsNotMinted and …Attempt1DoesNotMint.
• Upsert: making the upsert overwrite the stamp fails TestSavePurchaseExecution_FirstDispatchAtIsInsertOnly.
• RequestedAt: not passing the stamp as RequestedAt fails three Redshift dispatch tests. Letting a stamp-less Redshift rec default to now fails …RedshiftWithoutRequestedAtRefuses.
• Stamp step and attempt-0 error: skipping the stamp step fails three TestExecuteAndFinalize_Redshift* tests. Not marking an attempt-0 stamp error as before-submission fails …StampErrorFailsBeforeSubmission and …NilAtRefusalByAttempt.

Note for PR3a/3b (not blocking here)

The Retry successor builder (internal/api/handler_purchases.go:2775-2791) doesn't copy FirstDispatchAt from the failed execution. A Redshift successor therefore gets NULL, and stampRedshiftDispatch refuses it with "none can be minted". That fails closed, and Redshift retry is refused in this PR anyway. But TestStampFirstDispatch_RetrySuccessorKeepsCopiedStamp sets the stamp by hand, so when PR3a lifts the refusal, the builder must copy the stamp or the retry path stays refused.

Exact-head gate (Go 1.26.10)

• go build ./... and go vet on config, purchase and database: clean.
• go test ./internal/config ./internal/purchase ./internal/database/postgres/migrations: all ok.
• go test -tags integration (Postgres testcontainers): config ok (49s), migrations ok (134s), purchase ok (35s).
• golangci-lint: purchase 0 issues. config has 1 prealloc in store_postgres_history_page_integration_test.go:141, a file this PR doesn't touch; the same line is on main (from fix(history): set cloud_account_id on every purchase_history insert #1244).

[cristim]

Merging at fe14519 (claude-fleet, author). The approvals from @cc-libs and @as-qa-2 were given at 3a6caba.

The only change since then is fe14519, chore(db): renumber the first_dispatch_at migration to 000154. It is a rename-only commit under the merge-time numbering policy, so the approvals stand. Main's highest migration was 000153 at merge time. The migration SQL is byte-identical; its test and one comment follow the new number.

Verified on the merged tree (origin/main 0015bea plus this head, via git merge-tree):

• build and vet;
• unit tests for config, purchase and migrations;
• TestMigration_0154_FirstDispatchAt;
• -tags=integration for config, migrations and purchase.

CI is green at this head. CodeRabbit is waived from the gate.

Prod: apply migration 000154 before the prod code deploy, and never down-migrate it while this code is live. Dev auto-migrates on merge.