[cristim]

Part of LeanerCloud/cloud-commitments-go#107. This is the platform side of the Redshift purchase attempt journal. The go side shipped in cloud-commitments-go #696 (contract) and #701 (Redshift flow), and is pinned here by #1817.

What

• Migration 000161, purchase_attempts. Keyed by (idempotency token, execution ID). It stores the attempt state (sending, purchased or outcome_unknown, exactly go's strings), the reservation ID, and a write-once offering ID. A CHECK makes a row purchased if and only if it has a reservation ID. The down migration drops the table.
• Store (store_postgres_attempts.go). PostgresStore implements common.AttemptJournal:

  • RecordAttempt validates the record, then does one autocommit upsert whose WHERE clause encodes go's transition table:

    • none to sending or purchased;
    • sending to purchased or outcome_unknown;
    • outcome_unknown to purchased;
    • purchased to purchased only with the same ID.

    Anything else, including identical repeats and a first row of outcome_unknown, is an error. The offering ID is written once.

  • PriorAttempts reads all attempts for a token.

  • Neither method runs inside a caller's transaction.

• Wiring.
  • ManagerConfig.AttemptJournal is set only on the post-DB manager, through a new pure postDBPurchaseManagerConfig. The pre-DB manager keeps a nil journal, which keeps go's existing window dedupe.
  • Each purchase gets the journal next to its idempotency token, with the per-account execution ID.
• A bought-but-unrecorded node is never silent.
  • When go returns a reservation ID together with an error, the rec keeps PurchaseID with Purchased=false. A Retry successor that fails without an ID keeps its predecessor's ID.
  • The failure email uses the outcome-unknown template whenever an unrecorded commitment is present: "the provider returned a commitment that CUDly could not record". It carries a per-rec line, "Redshift reserved node was bought but CUDly could not record it; verify it in the cloud provider's console before buying again", and never says "retry". The template choice can't be overridden by callers that set OutcomeUnknown directly.
  • The failure log marker gains unrecorded_commitment_ids= for deployments without email.

Tests and verification (local, macOS, throttled; branch merged with main incl. #1816 and #1239 at 12a6b74)

• Unit tests: go test for config, purchase, email, server and ctxutil pass. TestIdempotencyKeyCopyHasSingleWriter passes, so this change adds no IdempotencyKey writer.
• Integration tests: -tags integration for config (the 16 transition cases, an invalid record rejected before any SQL, PriorAttempts across executions, offering ID write-once), migrations (000158-000161 and the down path, on top of fix(recommendations): key stored recs, rec ID and purchase identity on RDS az_config (#408) #1239) and purchase pass.
• Deletion probes, 13 on the first round and 4 on the review fixes, each failing a named test. They cover:
  • the WHERE branches, the first-row guard and COALESCE;
  • Validate, the injection, the wiring and the aggregator;
  • the email flag, the template clause and line, and the label;
  • the marker filters.
• Review: adversarial review APPROVED at 46cfef44, and again at the fix commit (fix(purchase): record fan-out accounts skipped before they started #1798 interplay, marker test, label).
• Not verified: no real Redshift purchase was made, and none is authorized for verification.

Deploy order

Apply migration 000161 BEFORE deploying this code. With DB_AUTO_MIGRATE off, deploying the code first fails closed: the journal's PriorAttempts and RecordAttempt hit a missing table, so every tokenized Redshift purchase is refused before the send. That is an outage for Redshift buys, never a double buy. Other services are unaffected. #1239 (000158-000160) has already merged, so there is no migration-number conflict.

Ordering

This lands before #702 PR3b, which uses ManagerConfig.AttemptJournal. The migration is 000161 because #1239 takes 000158-000160.

🤖 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 43 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: feat/107-platform-attempt-journal-v2
• File: .coderabbit.yaml
• Commit: 12a6b74
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5dd1ca0c-9be9-40bf-94de-bb34564ce27c

Commits

Reviewing files that changed from the base of the PR and between 97c7d59 and 12a6b74.

Files selected for processing: 18

• internal/config/store_postgres_attempts.go
• internal/config/store_postgres_attempts_integration_test.go
• internal/database/postgres/migrations/000161_purchase_attempts.down.sql
• internal/database/postgres/migrations/000161_purchase_attempts.up.sql
• internal/database/postgres/migrations/000161_purchase_attempts_test.go
• internal/email/purchase_outcome_unknown.go
• internal/email/purchase_outcome_unknown_test.go
• internal/email/sender.go
• internal/email/template_renderers.go
• internal/purchase/aggregate_unrecorded_commitment_test.go
• internal/purchase/execution.go
• internal/purchase/failure_notify.go
• internal/purchase/failure_notify_test.go
• internal/purchase/failure_notify_writers_test.go
• internal/purchase/manager.go
• internal/purchase/requested_at_test.go
• internal/server/app.go
• internal/server/app_postdb_config_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]

APPROVED 12a6b74 by claude-qa-ui-4

Cold review against go#107 decision A, with the go contract pkg/common/attempt_journal.go at the pinned c83aef8b18a9.

• Store (internal/config/store_postgres_attempts.go):
  • RecordAttempt validates first (:48-50).
  • It inserts only the first row (:30-32).
  • It allows only the forward moves sending→purchased/outcome_unknown, outcome_unknown→purchased, and purchased→purchased with the same ID (:37-41). Any other write matches 0 rows and errors (:57-60).
  • The ReservationID only appears with purchased (Validate plus a CHECK).
  • OfferingID is write-once via COALESCE.
  • Writes autocommit on the pool, and PriorAttempts (:66-86) returns every execution's rows.
  • Two writers on one (token, execution) are serialized by the PK and ON CONFLICT. No s.db read happens inside a caller's tx.
• Migration 000161: the state CHECK is exactly sending|purchased|outcome_unknown, with reservation-iff-purchased, PK (idempotency_token, execution_id) and non-empty IDs. The down migration is a documented lossy DROP. Main ends at 000160, so there is no clash.
• Wiring: the post-DB Manager gets AttemptJournal: pgStore (internal/server/app.go:875, via postDBPurchaseManagerConfig at :954), and NewManager keeps it (internal/purchase/manager.go:138). It reaches PurchaseOptions at execution.go:834 for single-account, fan-out (per-account ExecutionID), retries and re-drives; execution.go:1350 is the only PurchaseCommitment call. The pre-DB Manager (app.go:551) has no store and is replaced at reinit.
• Time: the PR adds no time.Now(). RequestedAt is the DB-stamped FirstDispatchAt (execution.go:758). The settle wait stays feat(api): gate Redshift retries and copy the first dispatch time #1816's 10 minutes on the DB clock and fix(purchase): refuse a Redshift dispatch inside the adoption window #1810's window stays 10 minutes; the journal's timestamps only order rows.
• Email: outcomeUnknownVariant (email/purchase_outcome_unknown.go:50-52), used by both senders, keeps the unrecorded-commitment variant from being replaced by the batched outcome-unknown text.
• Other services: only the redshift provider reads AttemptJournal. The PurchaseID-on-error change (execution.go:886-888) is inert where providers set CommitmentID only on success, so there are no new refusals.

Runs (GOMAXPROCS=4, -p 2):

• Build and vet, with and without integration, pass.
• Unit tests for config, purchase, email, server and migrations pass.
• Integration for purchase, config, email and the full migrations package (TestMigration_0161) passes.
• golangci-lint: one prealloc, in an untouched file.
• Probes: 14 of 16 killed:
  • journal not passed to the provider;
  • backward transition;
  • repeat of purchased with another ID;
  • ReservationID skipped;
  • the aggregator ID;
  • app time instead of the stamp;
  • the email variant (3);
  • app.go wiring dropped;
  • Validate skipped;
  • OfferingID overwrite;
  • first-row guard;
  • migration down.

Non-blocking, but please follow up (money path):

• Two wiring probes survived, so the journal can be silently disabled in production without a test failing: dropping attemptJournal: cfg.AttemptJournal in NewManager (manager.go:138), or nil-ing the config between postDBPurchaseManagerConfig and NewManager (app.go:954). Production then falls back to window dedupe. A test that builds the Manager through NewManager(postDBPurchaseManagerConfig(...)) and asserts the journal is set would pin it.
• purchase_attempts.execution_id has no FK to purchase_executions (000161 up.sql:17-27), so retention and account deletion leave journal rows forever. Keeping them is arguably safer; say so in the migration header and note the growth.
• idx_purchase_attempts_token (up.sql:29-30) duplicates the PK's leading column.
• A later write with a different OfferingID is ignored rather than refused (COALESCE).
• A failed successor's CommitmentID replaces the predecessor's PurchaseID (execution.go:886-888).
• A batched-path test with an unrecorded rec would pin the email variant end to end.

[cristim]

APPROVED 12a6b74 by claude-darling

Cold review; I did not write this change. This is a money path with a migration. It implements go#107 decision A on top of #702's claim and first_dispatch_at. There are three low follow-ups, and one of them (the deploy order) is worth a line in the body before merge.

Contract (go pinned at c83aef8; go.mod unchanged)

• States: sending, purchased and outcome_unknown (common/attempt_journal.go:13-17) match the CHECK in 000161.
• Transitions: INSERT … SELECT WHERE plus ON CONFLICT DO UPDATE … WHERE (store_postgres_attempts.go:29-41).
  • Zero rows affected returns an error (:57-60), matching the contract (attempt_journal.go:60-64).
  • Validation runs before the SQL (:48), and the write is committed by autocommit before returning.
• Order: the fix(purchase): Redshift reserved-node purchases without tag-based idempotency #702 stamp comes first (redshift_lineage.go:84-104), then journalPreflight (go dedupe.go:166-191), then recordSending (:251-264). If no record is written, nothing is bought.
• Reservation ID: saved before success is reported. If the purchased write fails, the result becomes ErrOutcomeUnknown and keeps the CommitmentID (dedupe.go:280-289).
• Same token, same execution: the primary key routes a second sending into the conflict path, which refuses it.
• Across executions: by contract the journal is a record, not a lock. The fix(purchase): Redshift reserved-node purchases without tag-based idempotency #702 tuple lock and conflict gate serialize lineages.

Migration 000161

• Up:
  • NOT NULL and non-empty CHECKs;
  • primary key (token, execution_id);
  • a CHECK that purchased holds exactly when reservation_id is set;
  • an index on the token.
• Down: DROP TABLE IF EXISTS, which loses the journal on purpose, as the file says.
• Number: main stops at 000160, so there's no collision.
• Tenant scope: there are no tenant columns, as with executions. The token is sha256 of a per-account lineage key (execution.go:947), so reads can't cross accounts.
• CI: covers up and down. TestMigration_0161_PurchaseAttempts passes in "Integration Tests (migrations, rest)", and the "Database migration down gate" passes.

One time source. The PR adds no time.Now. The dispatch time is the DB first_dispatch_at stamp (execution.go:757-758). The settle wait and the horizon use DB now() (store_postgres_redshift_retry.go:26-29). The time.Now fallback in withDispatchTime (redshift_lineage.go:155-163) is non-Redshift only.

Non-Redshift unchanged. Only the go redshift package reads AttemptJournal; I checked pkg, aws, azure and gcp. No other service returns an error together with a CommitmentID, so the new aggregator branch (execution.go:886-888) can't fire for them. A nil journal keeps the window dedupe, and the pre-DB manager has a nil journal.

Email precedence. outcomeUnknownVariant() is OutcomeUnknown || UnrecordedCommitment, and both the subject and the body use it (purchase_outcome_unknown.go:40-52, template_renderers.go:114). buildPurchaseFailedData sets both flags when a PurchaseID exists (failure_notify.go:156-161). Only the batched path overwrites OutcomeUnknown, and the variant still holds there. The "retry manually" template is never chosen for these.

Double-buy. Redshift redrive is still refused (manager.go:694). A same-execution re-dispatch is refused (dedupe.go:235-238). The Retry-successor adoption residual is the existing known_issues 39. No new path.

Findings (low)

1. Deploy order. Nothing says to apply 000161 before the code ships, and DB_AUTO_MIGRATE defaults to false. If the code goes first, PriorAttempts fails and every Redshift buy is refused. That fails closed, but it's an outage. Please add a deploy note to the body, as fix(purchase): persist the Redshift first-dispatch time and pass it as RequestedAt #1762 did for 000154.
2. Test gap. With the variant reduced to OutcomeUnknown only, the purchase-package tests stay green: no purchase-level test covers the batched path (failure_notify.go:126) with an unrecorded commitment. The email-package test does catch the same mutation.
3. Nit. The reasons for having no foreign key and no retention aren't documented: rows outlive their executions, which preserves the double-buy evidence. A data-warehouse rec gets the generic "Commitment X" line instead of "Redshift reserved node".

Probes (scoped with -run, each reverted, tree clean after)

• Killed:
  • the journal dropped from dispatch;
  • the journal dropped from the post-DB wiring;
  • RequestedAt = time.Now();
  • the primary key dropped;
  • the transition WHERE accepting everything;
  • the variant reduced (email package);
  • the flags not set;
  • the aggregator dropping the ID.
• Survived: the variant reduced (purchase package), as finding 2.

CI at this head: 41 of 41 pass, including Unit Tests, Integration (config, purchase, migrations rest, server, api) and the migration down gate. Run https://github.com/LeanerCloud/cloud-commitments-platform/actions/runs/42302937370.

[cristim]

Merge gate at 12a6b74:

• Approvals at this SHA: claude-darling and claude-qa-ui-4.
• CI and threads: CI is green (41/41) and there are 0 unresolved threads.
• Combo re-test (local, macOS, throttled): this head merged onto current main a8b8031. Migrations 000158-000160 (from fix(recommendations): key stored recs, rec ID and purchase identity on RDS az_config (#408) #1239) and this PR's 000161 are in order.
  • go build ./... and go vet pass.
  • Unit tests for purchase, config, email and server pass.
  • Integration tests pass for config (Attempt), migrations (Migration_016|Down) and purchase (Execut|Unrecorded|Redshift|Retry|Notify).
• Deploy order: see the body. Apply 000161 before the code. Code-first fails closed for Redshift buys.