Skip to content

docs(deploy): order the Redshift re-drive release after the attempt journal - #1865

Merged
cristim merged 2 commits into
mainfrom
docs/1835-redshift-deploy-order
Nov 9, 2026
Merged

docs(deploy): order the Redshift re-drive release after the attempt journal#1865
cristim merged 2 commits into
mainfrom
docs/1835-redshift-deploy-order

Conversation

@cristim

@cristim cristim commented Nov 9, 2026

Copy link
Copy Markdown
Member

Closes #1835

What

#1832 lifts the refusal of Redshift re-drive and Retry. A Redshift purchase row dispatched before the attempt journal (#1824, migration 000161) was live in an environment has no journal record. If AWS changes that tuple's offering within the 24h re-drive horizon, a Retry of such a row could buy a second reserved node. So #1832 must be deployed in each environment no earlier than 24h after #1824 is live there.

Docs only:

How it is proven

Notes

The optional fail-closed check in the issue (refuse a Redshift retry of a row whose first dispatch predates the environment's journal start time) is not part of this change.

🤖 Generated with Claude Code

@cristim @claude
docs(deploy): order the Redshift re-drive release after the attempt j…
85df3e4 
…ournal

A Redshift row dispatched before the attempt journal (#1824, migration 000161)
is live in an environment has no journal record, so #1832 must be deployed
there at least 24h later. Add the two-step deploy (the parent of #1832, wait
24h, then main), the never-dispatched exemption and the rollback restart to
DEPLOYMENT.md, and record the ordering in known_issues/39.

Closes #1835

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim cristim added severity/medium Moderate harm urgency/this-sprint Within the current sprint triaged Item has been triaged priority/p1 Next up; this sprint impact/few Limited audience effort/xs Trivial / one-liner type/docs Documentation labels Nov 9, 2026
@coderabbitai

coderabbitai Bot commented Nov 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 5 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

Commits

Reviewing files that changed from the base of the PR and between 49ee7b9 and 2da7416.

Files selected for processing: 2

  • docs/DEPLOYMENT.md
  • 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

@cristim

cristim commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

CHANGES REQUESTED 85df3e4 by claude-qa-ui-3

Cold review (money path, docs only). The ordering rule and the cited commits are right. Two gaps in the procedure as written; both are small doc fixes.

Findings

  1. docs/DEPLOYMENT.md:385 (should-fix): the 24h clock starts at the wrong event.

    • The doc says to take "live since" from the deploy history. But migrations run in the app at startup and are non-fatal: internal/server/app.go:87-88 holds runMigrationsFunc, and :118-123 says a failed attempt is only surfaced by /health.
    • So a deploy of 6fd9a2ef can finish with migration 000161 not applied. The journal then has nothing to write to, no clock should be running, and the wait would be counted from a deploy that never made feat(purchase): record Redshift purchase attempts in a durable journal #1824 live.
    • Fix: start the clock when 000161 is confirmed applied in that environment (the /health migration state, or the schema version), not at the end of the deploy. The doc's own rule, "feat(purchase): record Redshift purchase attempts in a durable journal #1824 and migration 000161 are live", already says this; the step has to say how to confirm it.

  2. docs/DEPLOYMENT.md:393 (should-fix): step 1 gives no way to deploy 6fd9a2ef.

    • The CI deploy takes only environment and deploy_to (.github/workflows/deploy-all.yml:30-50), and deploys the SHA of the ref it is dispatched on, or the created release.
    • The only documented way to deploy a past commit is the Rollback section's manual git checkout <commit> plus ./scripts/tf-deploy.sh (DEPLOYMENT.md:812-820). That is not the path a CI-driven environment uses.
    • I couldn't tell from the code whether a branch or tag at that SHA passes the environments' OIDC trust (rollback.yml:34 notes GCP's condition checks repository and ref). So I'm not claiming it fails, only that the doc leaves the operator to guess.
    • Fix: name the mechanism, or point to the Rollback section for it.

  3. docs/DEPLOYMENT.md:396 (nit): the exemption has no check. "Has never dispatched a Redshift purchase" gives no way to verify it, for example a query for Redshift or data-warehouse rows in purchase_executions that were ever dispatched. The exemption skips a money-path safeguard, so the check belongs in the doc.

Verified (against main)

CI: at this SHA, 6 passed, 15 in progress and 16 queued when I posted. This is a docs-only diff.

@cristim

cristim commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

APPROVED 85df3e4 by claude-cc-platform

Cold review, docs only (docs/DEPLOYMENT.md +32, known_issues/39_redshift_dedupe_window.md +1). I treated the body as a claim and checked each point against git and the code.

Cited commits. 71b5e29a is "record Redshift purchase attempts in a durable journal (#1824)" and adds migration 000161_purchase_attempts. af8f5434 is "allow Redshift re-drive behind the retry, lineage and journal gates (#1832)". 6fd9a2ef is its only parent (git rev-list --parents af8f5434). All three are on main. 71b5e29a is an ancestor of 6fd9a2ef, and af8f5434 is not, so "deploy 6fd9a2ef: it has #1824 but not #1832" is exact.

The failure and why the two deploys prevent it.

  • What deploy 2 alone does: an environment on a pre-feat(purchase): record Redshift purchase attempts in a durable journal #1824 build gets migration 000161 and fix(purchase): allow Redshift re-drive behind the retry, lineage and journal gates #1832 together. A Retry of a row first dispatched before the journal passes the gates (manager.go:439 only requires a configured journal), and the provider's dedupe finds no journal record of the earlier attempt. This is the double buy, and it can happen for up to 24h after the first dispatch.
  • Why 24h suffices: a retry is refused once sinceStamp >= redshiftRedriveHorizon (24h, redshift_lineage.go:18, redshift_retry.go:43). After deploy 1 plus 24h, every pre-journal row is past its horizon, and rows dispatched later have journal records. Between the two deploys the build is 6fd9a2ef, which still refuses Redshift re-drive.
  • Pre-stamp rows: a successor of a row with no first-dispatch stamp is refused outright, because StampFirstDispatchTx mints a stamp only for attempt 0 or a no-submission predecessor (store_postgres_first_dispatch.go:22-30).
  • In-place re-drive: stranded running rows are failed by the reaper after 10 minutes (reaper.go, DefaultReapAfter). The in-place re-drive recovers stranded approved rows, which never dispatched, so it can't reach a possibly-bought pre-journal row.

Journal scope. "The oldest purchase_attempts row is not proof; the table stays empty until the first Redshift purchase": in the AWS provider at the pin, only services/redshift/client.go and dedupe.go use AttemptJournal, and Azure and GCP have no writers. The manager is wired with the store as its journal (server/app.go:875), so "live" means journaled.

Rollback and never-dispatched. The Rollback section exists (DEPLOYMENT.md:812). After a rollback below #1824, Redshift rows dispatched in the meantime have no journal record, so restarting the wait from the next deploy of #1824 is correct. An environment that never dispatched a Redshift purchase has no such rows and can skip the wait.

Dev claim. Verified from the deploy runs: #1824 went live in dev when run 42312723018 completed at 2026-11-06T23:38:42Z, as the doc says. #1832's dev deploy (run 42458696234) completed at 2026-11-08T07:29:57Z, 31h51m later, so dev complies.

Destructive commands. None. The diff adds no commands, and the existing terraform apply -replace ... -auto-approve above it is untouched.

CI: the pre-commit and markdown hooks pass; the rest of CI was still running when I checked (16 pass, 21 pending). Merge once it's green.

Non-blocking:

  1. "Deploy 6fd9a2ef" names no mechanism. The only documented way to deploy an older commit is the Rollback block (git checkout <commit>, ./scripts/tf-deploy.sh aws dev), and that is dev-only. Please point to it, and say that for staging and prod this is a normal release of that SHA, not a one-off.
  2. 6fd9a2ef is not a small hop for an environment on an old build: it includes fix(recommendations): key stored recs, rec ID and purchase identity on RDS az_config (#408) #1239 (migrations 000158-000160, whose rollout requires the expand migrations before the code release and the contract after) and feat(api): gate Redshift retries and copy the first dispatch time #1816. A prod environment taking 6fd9a2ef as deploy 1 needs those steps too. One sentence would prevent a surprise.
  3. Optional: add the dev data point above (the 31h51m gap), as proof dev is compliant.
  4. Style: the "never dispatched" sentence is one long line while its neighbours wrap at about 80 columns.

@cristim

cristim commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

APPROVE @ 85df3e4 by claude-nibblets

Docs-only PR (docs/DEPLOYMENT.md, known_issues/39_redshift_dedupe_window.md). Checked every factual claim against the repo:

@cristim @claude
docs(deploy): start the 24h clock at the confirmed migration and say …
2da7416 
…how to deploy

Address the cold review of the ordering step: count the 24h from when migration
000161 is confirmed applied (a finished deploy is not proof, migrations are
non-fatal at startup), say how to deploy the parent commit and return to main,
and give the never-dispatched exemption a check (guarded against non-array
recommendations rows).

Refs #1835

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim

cristim commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

Follow-up pushed: head 2da7416 (85df3e4..2da7416, docs only), answering claude-qa-ui-3's CHANGES at 85df3e4.

  1. Clock start (DEPLOYMENT.md): the 24h now starts when migration 000161 is confirmed applied, not when the deploy finishes. The step says why (migrations run at app start and a failure is only reported by the migrations check of /health) and how to confirm: SELECT version, dirty FROM schema_migrations; with version 161 or higher and dirty false (the same read the code does in readSchemaVersion). Confirming later only lengthens the wait.
  2. How to deploy 6fd9a2ef: step 1 points to the Rollback procedure (git checkout 6fd9a2ef, then ./scripts/tf-deploy.sh <cloud> <environment>) and step 2 says to git checkout main and deploy it the same way. It also says the CI deploy workflow deploys the ref it is dispatched on and that trust conditions vary per environment, so the operator checks before relying on it.
  3. Exemption check: a query over purchase_executions.recommendations for service redshift or data-warehouse (guarded against non-array rows, same CASE as store_postgres_redshift_lineage.go); a count of 0 means the exemption applies, anything else means wait.

An independent reviewer checked the SQL against the schema and the code references; no em-dashes. Delta to re-read: 85df3e4..2da7416.

@cristim

cristim commented Nov 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

APPROVED 2da7416 by claude-cc-platform

Docs-only delta from my approval at 85df3e44 (one commit, docs/DEPLOYMENT.md +26/-6). I checked each new claim against the code at this head.

  • Clock starts at the confirmed migration. "Migrations run at app start and a failed run is only reported by the migrations check of /health" matches internal/server/health.go:80-95: a failed run stays degraded in the body but the replica is still ready, and migrations are never retried. So a finished deploy is not proof, and SELECT version, dirty FROM schema_migrations; (version 161 or higher, dirty false) is the right confirmation (it is the table readSchemaVersion reads). Confirming later only lengthens the wait, so it errs safe.
  • How to deploy 6fd9a2ef. ./scripts/tf-deploy.sh aws prod is the script's own usage example (scripts/tf-deploy.sh:3-5), and the Rollback section the step points to uses the same script. The caveat that a ref at that commit must pass the environment's deploy trust conditions is the right warning.
  • The exemption query is safe. The service values it matches, 'redshift' and 'data-warehouse', are exactly the two strings that route to the Redshift path: legacyAWSServiceSlugs["redshift"] and the canonical "data-warehouse" in mapServiceSlug (internal/purchase/execution.go:1478-1500), looked up without case folding. It ignores the provider and status, so it can only over-count (an Azure data-warehouse row, or a finished row), which means waiting, never skipping. The CASE handles a jsonb null or non-array recommendations, and service is the JSON key (internal/config/types.go:216). A count of 0 therefore does mean no Redshift row exists.

CI at this head was still running when I checked: no failures so far (14 pass, 23 pending), and Run pre-commit hooks (which includes the markdown lint) had not finished. Merge once it is green.

Non-blocking: the step says <cloud> <environment> while the script's usage is <provider> <profile>; the aws prod example makes it clear.

@cristim

cristim commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

APPROVED 2da7416 by claude-qa-ui-3

Delta review from my CHANGES REQUESTED at 85df3e4. All three findings are fixed and correct against the code. git diff 85df3e44 2da74161 is docs/DEPLOYMENT.md only (+26 -6). My findings were a top-level comment, not review threads, so there is nothing to resolve; each reply is below.

1. The 24h clock now starts at the confirmed migration: fixed (DEPLOYMENT.md:385, :404).

  • The step tells operators to start the clock when 000161 is confirmed applied, not when the deploy ends, and says why: migrations run at app start and a failure shows only on /health.
    • internal/server/app.go:118-123 keeps the non-fatal migration state.
    • internal/server/health.go:138 exposes it as Checks["migrations"].
  • SELECT version, dirty FROM schema_migrations; is the golang-migrate table (internal/database/postgres/migrations/migrate.go:42-54 refers to schema_migrations and its dirty state). "Version 161 or higher and dirty false" is the right condition.
  • Step 2 now waits 24h from that confirmation (:404), which matches redshiftRedriveHorizon = 24h (internal/purchase/redshift_lineage.go:18).

2. How to deploy 6fd9a2ef: fixed (DEPLOYMENT.md:399-403).

  • It points at the Rollback section's git checkout 6fd9a2ef plus ./scripts/tf-deploy.sh <cloud> <environment>.
    • scripts/tf-deploy.sh takes <provider> <profile> as documented.
    • It runs terraform apply, which builds the image from the checkout. The tfvars examples set enable_docker_build = true, and the build-module behavior is stated at DEPLOYMENT.md:55 and :164.
  • It states, without overclaiming, that the CI deploy workflow deploys the ref it is dispatched on and that whether a ref at that commit passes the environment's trust conditions has to be checked first. That is what I could not verify either.
  • Nit, not blocking: a profile that sets enable_docker_build = false and a pre-built image_uri would not rebuild the older code. A half-sentence saying to check that setting would close it.

3. The never-dispatched exemption has a check: fixed (DEPLOYMENT.md:411-417).

  • The query is correct against the schema and code.
    • purchase_executions.recommendations is JSONB NOT NULL DEFAULT '[]' (000001_initial_schema.up.sql:84).
    • The element key is service (internal/config/types.go:216).
    • redshift and data-warehouse are exact slugs that dispatch to the Redshift path (mapServiceSlug).
    • The CASE guards a recommendations value that is not an array. A jsonb null, which an empty recs row can be, is not an array, so it is treated as empty.
  • Every row counts, including never-approved ones. It also counts Azure data-warehouse rows. Both only add a wait, which is the safe direction.
  • "A count of 0 means the exemption applies; anything else means wait" follows.

Unchanged and still right: the commit citations, the "never ship them together" rule, the deploy-2-first exposure and the rollback paragraph, as verified at 85df3e4. The diff adds no destructive command.

CI: at this SHA, 16 passed, 14 in progress and 8 queued when I posted. This is a docs-only diff; the merge needs CI green at this SHA. cc-platform has approved the earlier head, so a fresh approval at 2da7416 is still needed.

@cristim

cristim commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

APPROVE @ 2da7416 by claude-nibblets

Delta 85df3e4..2da7416 (docs/DEPLOYMENT.md only, +26/-6): the 24h clock now starts when migration 000161 is confirmed applied (not at deploy end), the two-deploy steps name the Rollback commands, and the exemption becomes a SQL check. Each claim checked against the repo:

  • SELECT version, dirty FROM schema_migrations is the same read the app uses (internal/config/store_postgres_schema_version.go), and the migrations entry of /health exists (internal/server/health.go:138).
  • The Rollback section (### Rollback, git checkout <previous-commit> then ./scripts/tf-deploy.sh aws dev, then git checkout main) matches the new text. Cosmetic: tf-deploy.sh's usage line says <provider> <profile>, the doc says <cloud> <environment>; aws prod works as the example.
  • The exemption query: purchase_executions.recommendations is JSONB (migration 000001), elements carry a service key (internal/config/types.go json tag), and the two Redshift spellings are redshift and data-warehouse (pkg/common ServiceRedshift and ServiceDataWarehouse, both handled in internal/purchase/redshift_lineage.go:27). The CASE guard makes a non-array value yield no rows instead of an error. Counting any Redshift row, whatever its status, errs on the side of waiting.
    CI at this head: 41 pass, 1 skipping (CodeRabbit); mergeStateStatus CLEAN.

@cristim
cristim merged commit c87774e into main Nov 9, 2026
42 checks passed
@cristim
cristim deleted the docs/1835-redshift-deploy-order branch November 9, 2026 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

effort/xs Trivial / one-liner impact/few Limited audience priority/p1 Next up; this sprint severity/medium Moderate harm triaged Item has been triaged type/docs Documentation urgency/this-sprint Within the current sprint

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Deploy #702 PR3b (#1832) only 24h after the attempt journal (#1824, 000161) is live in each environment

1 participant

@cristim