Repository navigation
fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback - #1516
Conversation
…00059 re-applicable after a rollback FlipPurchaseRevocationInFlight was an unconditional UPDATE, so a stale concurrent Azure revoke reaching it after another request's MarkPurchaseRevoked set revocation_in_flight on a revoked row, where nothing clears it. The flip now only matches revoked_at IS NULL; a revoked row returns ErrPurchaseAlreadyRevoked (a missing row keeps its not-found error), and the handler answers already_revoked without posting a second Azure Return. Rolling back through 000059 and re-applying it failed: 000064's down is a no-op, so Purchaser stays at ...0007 and 000059's guard refused it as a foreign "Purchaser". 000059 now yields to that row (and restores its system_managed flag), and its admin backfill only targets ...0005 when that row really is Purchaser, so a re-apply no longer adds admins to Standard Users. Adds a 000059 down/up round-trip test and a down-to-version-1 round trip. No new migration. Refs LeanerCloud#316 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rF6. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
VERDICT: APPROVE 85d6884 Findings:
Attacked:
|
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rF8. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 3 approving independent review(s) on |
|
Independent review 2 (claude-fleet, cold): APPROVE 85d6884 Findings:
Attacked:
Verified locally:
Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged. |
LeanerCloud#1516 was squash-merged, so this branch's copies of its commits conflicted with main in the six files both PRs touch. Resolved to main plus this PR's own commit (a81a3c3): the merged tree is identical to cherry-picking a81a3c3 onto origin/main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Follow-up to #1268 for the two items @cristim kept open on #316 (comment). The A04-012 key-delimiter normalization is still waiting on an owner decision, so this PR uses Refs, not Closes.
Refs #316
1.
FlipPurchaseRevocationInFlightis now a compare-and-setRoot cause. The flip was an unconditional
UPDATE purchase_history SET revocation_in_flight = true WHERE purchase_id = $1. A stale revoke request could load the row before another request revoked it, then reach the flip after that request'sMarkPurchaseRevokedcommitted. The flip then set the flag again on a revoked row.MarkPurchaseRevokedis guarded onrevoked_at IS NULL, so nothing ever cleared it, and the row stayed inidx_purchase_history_revocation_in_flightreadingRevocationInFlight: true. The stale request also went on to post a second Azure Return for a reservation already recorded as returned.Fix.
internal/config/store_postgres.go: the UPDATE addsAND revoked_at IS NULL. When 0 rows match, an existence check sorts out the two cases. A missing row keeps the samepurchase_id %q not founderror as before (the not-found semantics cristim flagged are unchanged). A revoked row returns an error wrapping the newconfig.ErrPurchaseAlreadyRevoked. An unrevoked row that is already in flight still flips idempotently.internal/config/errors.go,internal/config/interfaces.go: the new sentinel, plus the updated contract.internal/api/handler_purchases_revoke.go: the flip moved intoflipRevocationInFlight. OnErrPurchaseAlreadyRevokedit posts no Azure Return and answersalready_revoked, the same response the handler's up-front idempotency check gives. It re-reads the row best-effort to fill inrevoked_atandrevoked_via. Every other flip error stays best-effort (log and continue), as before.Concurrency. The flip and
MarkPurchaseRevokedboth UPDATE the same row, so the row lock serializes them. Under READ COMMITTED, a flip that waited on the mark re-evaluatesrevoked_at IS NULLagainst the committed row and matches nothing. Whatever the order, the row ends revoked with the flag clear. The race test below exercises this on real Postgres.2. 000059 down/up round trip
Root cause. 000064's down is a deliberate no-op, so a rollback through 000059 leaves Purchaser at
...0007. Re-applying 000059 then hit its own guard ("a group named 'Purchaser' already exists with a different id") and stopped. Two more problems showed up once that was out of the way. 000059's down dropssystem_managed, and the re-added column defaulted the relocated Purchaser tofalse. And the admin backfill added every admin to whatever sat at...0005, which is Standard Users on every real database (#942). So a re-apply would add every admin created since to Standard Users.Fix (edits to the existing
000059_seed_purchaser_group.up.sql, no new migration):...0007, 000064's own row. A "Purchaser" under any other id still refuses. A new test pins this.Purchaserat...0007. On a forward run that row doesn't exist yet, so nothing changes.ON CONFLICT (id) DO NOTHINGbecomesON CONFLICT DO NOTHING. When...0005is free and Purchaser sits at...0007, the insert would otherwise abort ongroups_name_key. The guard above has already ruled out every other name conflict....0005is actually namedPurchaser. 000064 backfills the real Purchaser.As with #1268's 000027 edit, this changes an already-applied migration, so it only affects fresh databases and re-applies after a rollback. On a fresh database no users exist at 000059, so the narrower backfill changes nothing there.
Verification
Local Linux VM, Go 1.26.9. Everything ran with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true. "Integration" means real PostgreSQL 16 in Docker (testcontainers, the package's shared container). Handler tests use the mock store with stub Azure clients. Nothing calls a cloud API.Fail-before evidence: I swapped
origin/main's code back in (only the named function or file), ran the new tests, then restored.TestFlipPurchaseRevocationInFlight_CompareAndSet/a_revoked_row_is_refused_and_keeps_the_flag_clear(integration)…/stale_flips_racing_a_mark(integration; 20 rounds, each with 1MarkPurchaseRevokedand 4 concurrent flips on one in-flight row)…/an_unrevoked_row_still_flips,_idempotently,…/a_missing_row_is_still_not_found(integration, preservation)TestCallAzureReturn_FlipFindsRowRevoked/already_revoked:_no_Return,_already_revoked(unit, mock store)…/other_flip_error:_still_returns_and_marks(unit, preservation)TestMigration_SeedPurchaserGroup_DownUpRoundTrip/Standard_Users_at_...0005(integration)migration 000059: a group named 'Purchaser' already exists with a different id…/...0005_free(integration)TestMigrations_DownToOneRoundTrip(integration; down to version 1, then back up to head)TestMigration_SeedPurchaserGroup_ForeignPurchaserStillRefused(integration, preservation of the guard)TestMigration_SeedPurchaserGroup_Down(#1268's tests),TestMigration_RelocatePurchaserGroup(integration)Mutation probes on the 000059 fix (each reverted afterwards):
name = 'Purchaser'backfill guard: the round trip fails with "the re-applied backfill must not add an admin to Standard Users"....0007system_managedrestore: the round trip fails on the group snapshot.ON CONFLICT (id):...0005_freefails withduplicate key value violates unique constraint "groups_name_key".ForeignPurchaserStillRefusedfails.I didn't run the other integration tests in the touched packages (whole packages,
-race, git-secrets). Per the queue process, those run on the combined tree.Combined-tree verification at local queue commit
2be43e1e= origin/mainb9271e88+fix/issue-363-next,fix/issue-177-next,fix/issue-919-next,fix/issue-313-next,fix/issue-532-nextandfix/issue-316-nextmerged in that order. Passed:go build ./...;go vetwith and without-tags integration;go test -race -short -count=1 ./...; golangci-lint v2.10.1 with and without the integration tag; gocyclo -over 10;go test -race -tags integrationoninternal/api,internal/auth,internal/config,internal/database/postgres/migrations,internal/purchase,internal/serverandinternal/testutil; the git-secrets scan andscripts/test-git-secrets-allowlist.sh.Status of #316 after this PR
DerivePlanProvidersMarkPurchaseRevokedclears the flagOut of scope (noted, not changed)
ClearRevocationInFlightis still unconditional. After a failed Return it clears the flag even on a revoked row, which is the right end state, so I left it.AND revocation_in_flight = false) would stop that. But the loser would then have to answer "in flight" for a Return that might still fail, and the flip is documented as idempotent for an in-flight row. That's a design choice for the owner, not part of this follow-up.MarkPurchaseRevokedstill key onpurchase_idalone. The unique key is(provider, account_id, purchase_id). This is pre-existing and unchanged.Frontend follow-up for the UI owner
None required.
already_revokedis an existing, documented response of the revoke endpoint (the up-front idempotency check already returns it). This PR only returns it in one more race window.Maintainer: please mirror the issue's triage labels onto this PR:
priority/p3,severity/low,urgency/eventually,impact/internal,effort/m,type/bug.🤖 Generated with Claude Code