Repository navigation
fix(db): contract the canceled rename: normalize, narrow the status CHECKs, stop using cancelled_by - #1253
Draft
IAmJSD wants to merge 1 commit into
Draft
fix(db): contract the canceled rename: normalize, narrow the status CHECKs, stop using cancelled_by#1253IAmJSD wants to merge 1 commit into
IAmJSD wants to merge 1 commit into
Conversation
…HECKs, stop using cancelled_by Migration 000144 is the contract half of 000089. Under a table lock it normalizes every legacy 'cancelled' status in purchase_executions and ri_exchange_history, drains legacy-only actors into canceled_by, and narrows both status CHECKs to the 000089 lists minus 'cancelled'. The code stops reading and writing purchase_executions.cancelled_by: reads project canceled_by, the cancel transition and SetCancelledBy write only canceled_by, and SavePurchaseExecution writes canceled_by, keeping an actor already on the row (the value reads saw before). The LegacyStatusCanceled constant and every dual-spelling branch are gone. The cancelled_by column is kept. The previous build still reads COALESCE(canceled_by, cancelled_by) and writes cancelled_by, and it keeps serving during a rolling deploy and after a code-only rollback, so dropping the column in the same release would break every execution read on it. A trigger mirrors any legacy-only actor write into canceled_by until a follow-up migration drops the trigger and column. Refs LeanerCloud#47 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is the contract half of the 000089
cancelled->canceledrename, minus the column drop. The column drop can't safely ship in the same release, so it comes in a follow-up (explained below).What was still open on main. 000089 (EXPAND, #1277) widened both status CHECKs, added
canceled_byand deferred everything else. Nothing after it touches these constraints or the column; I checked every migration through 000134 plus the open PRs holding 000135-000143. Main still:config.LegacyStatusCanceledappears in about ten switches and lists, and the cleanup SQL usesstatus IN ('cancelled','canceled').COALESCE(canceled_by, cancelled_by).cancelled_by:SavePurchaseExecutionwrites onlycancelled_by(nevercanceled_by).transitionExecutionStatusandSetCancelledBydual-write both columns.Nothing on main writes a
'cancelled'status topurchase_executionsorri_exchange_history. The remaining double-l literals belong toladder_runs/ladder_tranches/ AWS listing state, which are different tables and out of scope.Changes
Migration 000144
contract_canceled_spelling(up), one transaction:LOCK TABLE purchase_executions, ri_exchange_history IN ACCESS EXCLUSIVE MODE. The CHECK rebuild below takes this lock anyway. Taking it first means no writer can commit between the normalize/drain and the narrowed CHECKs, or between the drain and the trigger.purchase_executions_mirror_legacy_cancel_actor,BEFORE INSERT OR UPDATE ... WHEN (NEW.canceled_by IS NULL AND NEW.cancelled_by IS NOT NULL). It setscanceled_by := cancelled_by, socanceled_byalways equalsCOALESCE(canceled_by, cancelled_by), which is exactly what older builds read.status 'cancelled' -> 'canceled'in both tablescanceled_bypurchase_executions_status_checkandri_exchange_history_status_checkas the 000089 lists minus'cancelled'. The test checks the new constraint text is the old one with exactly that one entry removed.Down: widens both CHECKs back to the exact 000089 definitions, drops the trigger and function, and backfills
cancelled_byfromcanceled_bywhere it is NULL. Rows staycanceled. The widened CHECKs accept both spellings and every build since #1277 reads both, so there is no need to convert data back.Code (no read or write of
purchase_executions.cancelled_byremains):canceled_by.SetCancelledBywrite onlycanceled_by.SavePurchaseExecutioninsertscanceled_byand on conflict setscanceled_by = COALESCE(purchase_executions.canceled_by, $17).COALESCE(canceled_by, cancelled_by), so a save could never change an actor already incanceled_by.$17straight intocanceled_bywould let a save from a stale copy (read before a concurrent cancel) erase that cancel's actor.LegacyStatusCanceledis removed, along with every dual-spelling branch: the history filter/annotation/KPI, cancel-recovery normalization, plan health, ramp settled/terminal/health-scored lists, ladder reconcile/resolve, ramp baseline and schedule clock. The cleanup query is nowstatus = 'canceled'.'cancelled'row can no longer be stored. The 000089 test keeps its//nolint:misspellbecause it still exercises the legacy spelling at version 89.Why
cancelled_byis not dropped here (deploy order)Migrations run at app startup (
ensureDB->RunMigrations). So the first instance of the release that ships 000144 applies it while instances of the previous build (P) are still serving.rollback.ymlalso redeploys an older image without touching the schema. P readsCOALESCE(canceled_by, cancelled_by)in every execution query and writescancelled_by. If this release dropped the column, every execution read on P would fail with "column does not exist" for the rest of the rollout, and so would any code-only rollback. So:cancelled_by'cancelled'(verified on main), so the narrowed CHECKs don't affect it. P's legacy-column writes are mirrored by the trigger, so R can readcanceled_byalone. P's COALESCE reads still work, since the column exists.DROP COLUMN cancelled_byThat follow-up is the rest of #47, which is why this PR says Refs, not Closes. It needs a follow-up issue (or #47 left open) to track it.
Rollback story with the DB at 000144
03c0ac9d) is safe without a schema change:canceled;canceled_by.'cancelled'and the narrowed CHECK rejects it. That needs 000144.down then 000089.down, which is already the requirement for going below feat(api): page the History view with a keyset cursor (has_more, next_cursor) #1277.000144.downalone puts the schema back to the 000089 state and is safe with R or P code running.Failure mode: migrations fail open (
ensureDBlogs and keeps serving). If 000144 failed, R would run on the unnormalized schema: surviving'cancelled'rows and any un-drained legacy-only actors would read as unknown status or missing attribution. Nothing breaks outright, and/healthreports the failed migration. The migration is a few UPDATEs and two CHECK rebuilds on two tables under one lock, so the main risk is lock wait on a busy table.Lock ordering: 000144 locks
purchase_executionsthenri_exchange_history. I found no app transaction that writes both, so I don't expect deadlocks. If one happened, Postgres aborts one side; for the migration that means a rolled-back transaction and the existing dirty-flag recovery.Refs #47
Verification
Run on this branch (
62feb4fc), Go 1.26.9,AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null.go build ./...,go vet ./...,go vet -tags integration ./...: clean.-count=1):./internal/config,./internal/api,./internal/purchase,./internal/mocks,./internal/database/postgres/migrations: all ok.golangci-lint run --allow-parallel-runners(v2.10.1), with and without--build-tags integration, on./internal/config/... ./internal/api/... ./internal/purchase/... ./internal/database/postgres/migrations/... ./cmd/cleanup-lambda/...: 0 issues.gocyclo -over 10 -ignore "_test\.go"on every changed non-test file: nothing over 10.New tests
TestPGXMock_ExecutionCancelActorUsesOnlyCanceledBy(pgxmock, unit). Its query matcher rejects any statement naming the legacy column. It drivesGetExecutionByID,TransitionExecutionStatus(... canceled),SetCancelledByandSavePurchaseExecution, and pins theCOALESCE(purchase_executions.canceled_by, $17)save.store_postgres.goswapped back toHEAD~1, it FAILS (statement still references the legacy cancel actor column).TestMigration_000144_ContractCanceledSpelling(integration, real Postgres 16 via testcontainers). FrompreviousMigrationVersion(t, 144)it seeds legacy-status, drain-only, both-columns and conflicting-actor rows plus a legacyri_exchange_historyrow, then migrates to 144 and asserts:canceled_by, and a legacy write never overrides an existingcanceled_by;TestMigration_000144_ConcurrentLegacyWritersKeepAttribution(integration, race). Four writers insert legacy-only actor rows (the shape an older build's save produces) continuously before, during and afterMigrateToVersion(144). It requires at least 20 inserts after the migration, no writer errors, and zero rows withcancelled_byset andcanceled_byNULL (about 1,260 rows in the run).Mutation probes (each reverted afterwards):
ContractCanceledSpellingFAILS (a legacy-only insert must be mirrored into canceled_by);ConcurrentLegacyWriters...FAILS (every legacy-only actor must be in canceled_by, of 1264 rows)ContractCanceledSpellingFAILS on the drain-only rowpurchase_executionsstatus normalization removedcheck constraint "purchase_executions_status_check" ... is violated by some row (SQLSTATE 23514)Changed integration tests, run narrowly:
TestPersistExecution_RetryAfterRowLeftPendingfailed once in the first combined run on a loaded machine, and the error detail was not captured. It passed on two reruns, alone and together with the other two tests.TestRampRetention_OldCleanupWritersPreserveHeldRowsneeded its expected delete count lowered from 3 to 2 once the legacy fixture row was removed.All of the above is local evidence: unit tests, pgxmock and testcontainers Postgres. Nothing was run against a deployed environment.
Combined-tree verification. This branch was tested together with six other queued branches on a local, never-pushed tree:
queue/platform@77d116c6= origin/maina14dc8f2plus these heads, merged in this order:Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....--build-tags integration, andgocyclo -over 10is silent on the changed non-test files. These ran atb61a9c30, which differs from77d116c6only by one.gitallowedline on fix/issue-1233.-tags integration -count=1):cmd/cleanup-lambda,internal/analytics,internal/auth,internal/database/postgres/migrationsandinternal/purchase.internal/apipasses, except two tests whose Postgres container didn't start, or whose migrations hit a timeout, under machine load. Rerun alone on the same tree, both pass:TestDeleteManagedPlanByExecutionStatusandTestNotifiedRow_RescheduleDoesNotOverwriteConcurrentCancel.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).TESTCONTAINERS_RYUK_DISABLED=trueto avoid a testcontainers reaper race under load.scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope
DROP COLUMN cancelled_byfollow-up, described above. Its migration should also drop the trigger and function and do a final drain.ramp_ledger_holds()(000124) still lists both spellings. That is harmless: the function is IMMUTABLE SQL, and redefining it is churn for no behavior change now that'cancelled'can't be stored.internal/mocks/stores.godefault returns andhandler_ri_exchange_test.goTestApproveRIExchange_AlreadyCancelled.ladder_runs.cancelled_by/ ladder'cancelled'statuses are a separate table and spelling (000080) and are not part of this rename.PurchaseExecution.CancelledBy(JSONcancelled_by, which the frontend consumes) and the method nameSetCancelledByare unchanged.Frontend follow-up for the UI owner
After 000144 the API never returns
cancelledfor purchase executions or RI exchanges, so the dual-spelling branches added for 000089 can go:frontend/src/history.ts: status badge (case 'cancelled'), the status counts (s === 'canceled' || s === 'cancelled') and the two filter predicates around lines 1187 and 1207. The'cancelled'chip key itself is UI-internal and can stay.frontend/src/riexchange.tsaround line 2363:case 'cancelled'in the exchange status badge.None of this is required for correctness. It is dead code once 000144 has run.
Maintainer: please mirror the issue's triage labels onto this PR:
priority/p2,severity/medium,impact/internal,type/chore,triaged,urgency/this-quarter,effort/s.🤖 Generated with Claude Code