Skip to content

fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback - #1516

Merged
IAmJSD merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-316-next
Oct 26, 2026
Merged

IAmJSD merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-316-next

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

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. FlipPurchaseRevocationInFlight is now a compare-and-set

Root 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's MarkPurchaseRevoked committed. The flip then set the flag again on a revoked row. MarkPurchaseRevoked is guarded on revoked_at IS NULL, so nothing ever cleared it, and the row stayed in idx_purchase_history_revocation_in_flight reading RevocationInFlight: 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 adds AND revoked_at IS NULL. When 0 rows match, an existence check sorts out the two cases. A missing row keeps the same purchase_id %q not found error as before (the not-found semantics cristim flagged are unchanged). A revoked row returns an error wrapping the new config.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 into flipRevocationInFlight. On ErrPurchaseAlreadyRevoked it posts no Azure Return and answers already_revoked, the same response the handler's up-front idempotency check gives. It re-reads the row best-effort to fill in revoked_at and revoked_via. Every other flip error stays best-effort (log and continue), as before.

Concurrency. The flip and MarkPurchaseRevoked both UPDATE the same row, so the row lock serializes them. Under READ COMMITTED, a flip that waited on the mark re-evaluates revoked_at IS NULL against 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 drops system_managed, and the re-added column defaulted the relocated Purchaser to false. 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):

  • The foreign-Purchaser guard also allows ...0007, 000064's own row. A "Purchaser" under any other id still refuses. A new test pins this.
  • The system-managed UPDATE also restores the flag on Purchaser at ...0007. On a forward run that row doesn't exist yet, so nothing changes.
  • ON CONFLICT (id) DO NOTHING becomes ON CONFLICT DO NOTHING. When ...0005 is free and Purchaser sits at ...0007, the insert would otherwise abort on groups_name_key. The guard above has already ruled out every other name conflict.
  • The admin backfill runs only when ...0005 is actually named Purchaser. 000064 backfills the real Purchaser.
  • The down file's "not reversed" note now says the stray Standard Users memberships came from the old up.

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.

export PATH=$HOME/.local/go/bin:$HOME/go/bin:$HOME/.local/bin:$PATH
go build ./... ; go vet ./... ; go vet -tags integration ./...                                   # clean
go test -count=1 ./internal/config/ ; go test -count=1 ./internal/api/ ; go test -count=1 ./internal/database/postgres/migrations/ ; go test -count=1 ./internal/mocks/   # all ok
golangci-lint run --allow-parallel-runners [--build-tags integration] ./internal/config/ ./internal/api/ [./internal/database/postgres/migrations/]   # 0 issues (both)
gocyclo -over 10 -ignore "_test\.go" <changed .go files>                                        # silent
go test -tags integration -count=1 -timeout 20m -run 'TestFlipPurchaseRevocationInFlight_CompareAndSet|TestMarkPurchaseRevoked_ClearsRevocationInFlight' ./internal/config/
go test -tags integration -count=1 -timeout 20m -run 'TestMigration_SeedPurchaserGroup|TestMigrations_DownToOneRoundTrip|TestMigration_RelocatePurchaserGroup' ./internal/database/postgres/migrations/
go test -count=1 -run 'TestCallAzureReturn_FlipFindsRowRevoked|TestRevokePurchase' ./internal/api/

Fail-before evidence: I swapped origin/main's code back in (only the named function or file), ran the new tests, then restored.

Test Before (main's code) After
TestFlipPurchaseRevocationInFlight_CompareAndSet/a_revoked_row_is_refused_and_keeps_the_flag_clear (integration) FAIL: no error, flag set on the revoked row PASS
…/stale_flips_racing_a_mark (integration; 20 rounds, each with 1 MarkPurchaseRevoked and 4 concurrent flips on one in-flight row) FAIL in round 0: "a flip that lost to the mark must not re-flag the revoked row" PASS
…/an_unrevoked_row_still_flips,_idempotently, …/a_missing_row_is_still_not_found (integration, preservation) PASS PASS
TestCallAzureReturn_FlipFindsRowRevoked/already_revoked:_no_Return,_already_revoked (unit, mock store) FAIL: Return posted and row marked again PASS
…/other_flip_error:_still_returns_and_marks (unit, preservation) PASS PASS
TestMigration_SeedPurchaserGroup_DownUpRoundTrip/Standard_Users_at_...0005 (integration) FAIL: migration 000059: a group named 'Purchaser' already exists with a different id PASS
…/...0005_free (integration) FAIL: same refusal PASS
TestMigrations_DownToOneRoundTrip (integration; down to version 1, then back up to head) FAIL: same refusal at 000059 PASS
TestMigration_SeedPurchaserGroup_ForeignPurchaserStillRefused (integration, preservation of the guard) PASS PASS
TestMigration_SeedPurchaserGroup_Down (#1268's tests), TestMigration_RelocatePurchaserGroup (integration) PASS PASS

Mutation probes on the 000059 fix (each reverted afterwards):

  • Drop the name = 'Purchaser' backfill guard: the round trip fails with "the re-applied backfill must not add an admin to Standard Users".
  • Drop the ...0007 system_managed restore: the round trip fails on the group snapshot.
  • Put back ON CONFLICT (id): ...0005_free fails with duplicate key value violates unique constraint "groups_name_key".
  • Make the guard never fire: ForeignPurchaserStillRefused fails.

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/main b9271e88 + fix/issue-363-next, fix/issue-177-next, fix/issue-919-next, fix/issue-313-next, fix/issue-532-next and fix/issue-316-next merged in that order. Passed: go build ./...; go vet with 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 integration on internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase, internal/server and internal/testutil; the git-secrets scan and scripts/test-git-secrets-allowlist.sh.

Status of #316 after this PR

Item Status
A04-010 seed-group rollbacks Fixed in #1268
A04-011 000027 partition-safe dedupe Fixed in #1268
A04-012 fail-open DerivePlanProviders Fixed in #1268
A04-012 key-delimiter normalization migration Open: owner decision, not touched here
A04-013 MarkPurchaseRevoked clears the flag Fixed in #1268
Follow-up 1: flip not compare-and-set This PR
Follow-up 2: 000059 down/up round trip This PR

Out of scope (noted, not changed)

  • ClearRevocationInFlight is 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.
  • Two first-time revokes that both read the row unrevoked and unflagged can both pass the flip and both post a Return. Azure rejects the second as already returned. Making the flip a real claim (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.
  • The flip and MarkPurchaseRevoked still key on purchase_id alone. 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_revoked is 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

…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>
@IAmJSD IAmJSD added impact/internal Team-internal only triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/m Days type/bug Defect labels Oct 26, 2026
@coderabbitai

coderabbitai Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 85d688403cf1a3b4f9081816804e790f84b8d497

Blocking:

  • none

Non-blocking:

  • Pre-existing and not introduced here, but adjacent and on a money path. Two first-time revokes can both pass the flip, as the body says. When Azure rejects the second Return, handleAzureReturnError calls ClearRevocationInFlight (internal/api/handler_purchases_revoke.go:905) while the first request may sit between its successful Return and MarkPurchaseRevoked. If the first request's mark then exhausts its retries, the row is left unrevoked and unflagged. The finalize sweep doesn't see it, and the user is told RECONCILE_PENDING. A claim-style flip (AND revocation_in_flight = false) plus "in flight" for the loser would close it. That's the owner decision the body defers, and I'd recommend it as a follow-up issue.
  • The flip, the existence check and MarkPurchaseRevoked all still key on purchase_id alone, not (provider, account_id, purchase_id). This is pre-existing and acknowledged in the body.

Attacked:

  • Concurrency: the flip and the mark UPDATE the same row, so under READ COMMITTED a flip blocked on the mark re-evaluates revoked_at IS NULL and matches 0 rows. The existence check after a 0-row update tells the two cases apart: a missing row is not-found, a revoked row is ErrPurchaseAlreadyRevoked. A delete in between degrades to not-found, which is harmless. The already_revoked early return skips both Return and Mark. Every other flip error stays best-effort, as before.
  • Migration 000059 edit: an applied migration is changed, which is the same precedent as fix(config/db): seed-group rollbacks, partition-safe 000027 dedupe, fail-closed plan providers, revocation_in_flight clear #1268, and no checksum pins migration files. On a fresh database app.admin_email is never set by RunMigrations, so 000005 creates no user and the narrower admin backfill changes nothing. The bare ON CONFLICT DO NOTHING can only swallow a name conflict at ...0007, because the DO guard already refuses any other foreign "Purchaser".
  • Fail-before, 3 checks, each restored afterwards:
    • Reverted the handler to main: TestCallAzureReturn_FlipFindsRowRevoked/already_revoked fails.
    • Removed AND revoked_at IS NULL: TestFlipPurchaseRevocationInFlight_CompareAndSet fails both the revoked-row subtest and the race subtest.
    • Reverted 000059...up.sql to main: both DownUpRoundTrip subtests and TestMigrations_DownToOneRoundTrip fail with the 000059 "already exists with a different id" refusal.
  • Ran: go build ./...; go vet ./... with and without -tags integration; go test -race -short -count=1 on api, config and migrations (all ok); go test -race -tags integration -count=1 on config, migrations and api (all ok); golangci-lint v2.10.1 with and without the integration tag (0 issues); gocyclo -over 10 on the changed files (silent).

Body-stale:

  • none found

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.

@cristim

cristim commented Oct 26, 2026

Copy link
Copy Markdown
Member

VERDICT: APPROVE 85d6884
PR #1516 is OPEN. One commit, no merges. It adds no new migration and edits only 000059. No other open PR touches 000059 or 000064.

Findings:

  • nit PR body ("On a fresh database no users exist at 000059"): this is not strictly true. 000005/000006 seed an admin user when the app.admin_email GUC is set. No code in the repo sets it, but an operator could. On such a fresh DB, the old 000059 added that admin to Standard Users (the perf(config): index the active purchase-history expiry filter #942 bug), and the new one does not. That change is a fix, not a regression (admins already hold admin:*), so the claim only needs rewording.
  • nit internal/api/handler_purchases_revoke.go:836: if the best-effort re-read fails, the response is already_revoked with revoked_at: "". I grepped frontend/src: nothing branches on already_revoked or parses revoked_at from this response, so there is no visible effect.
  • nit internal/config/store_postgres.go:3002-3009: the existence-check tail duplicates MarkPurchaseRevoked:2959-2969. The duplication is small and consistent between the two, and not worth a helper.
  • Residual gaps the PR body itself lists, which I confirmed and which are pre-existing, not introduced here:
    (a) Two first-time revokes that both load the row unflagged both pass the flip. The flip is still idempotent on an in-flight row (RowsAffected=1 when setting the same value), so both post a Return.
    (b) ClearRevocationInFlight is unconditional (handler:905). If A's Return fails before B's Mark commits, A clears B's flag, and the sweep then loses the row should B's Mark fail.
    (c) Flip, Mark and the re-read key on purchase_id alone, not (provider, account_id, purchase_id).
    The CAS closes exactly the window chore(config/db): low correctness findings from the 2026-09-02 audit #316 follow-up 1 describes (a flip after Mark commits).

Attacked:

  • CAS correctness under READ COMMITTED: the flip and MarkPurchaseRevoked UPDATE the same row, so the row lock serializes them. A flip blocked behind Mark re-evaluates revoked_at IS NULL on the committed tuple (EvalPlanQual), matches 0 rows, and the existence SELECT then sees the committed revoked row, so it returns ErrPurchaseAlreadyRevoked.
    • Flip before Mark: Mark clears the flag.
    • Row deleted between the UPDATE and the EXISTS: returns not-found, the old semantics.
    • Both statements run autocommit on the pool, not inside a caller transaction.
  • Lost-CAS reporting: the handler maps only errors.Is(ErrPurchaseAlreadyRevoked) to already_revoked with no Return and no Mark. The shape matches the up-front idempotency check at handler:193. Every other flip error still logs and continues, as before. The not-found text is unchanged.
  • Callers: FlipPurchaseRevocationInFlight has one production caller (handler:825). internal/mocks/stores.go is unchanged and compatible. Approve/execute operate on purchase_executions and never touch revoked_at or revocation_in_flight, so I found no interaction with either.
  • Migration 000059 edit, by DB state:
    • Existing DBs (version >= 59): golang-migrate never re-runs the file and keeps no checksum, so there is no effect. 000074 re-asserts only 059's schema, so it is unaffected.
    • Fresh DBs: 000057 holds ...0005 as Standard Users. The INSERT conflicts on id under both the old and new ON CONFLICT, and the backfill now skips. 000064 Case 2 then seeds ...0007 and backfills admins. The end state is the same except for the nit above.
    • ...0005 free and no Purchaser (000057 skipped the seed): 000059 inserts at ...0005 and backfills, then 000064 Case 1 relocates. This is unchanged.
    • Down to 58 and up again, with Purchaser at ...0007: the guard passes. The INSERT name-conflict is absorbed by the bare ON CONFLICT, which is safe because the guard has already ruled out every other Purchaser id. system_managed is restored on ...0007, the backfill skips, and 000064 Case 3 is idempotent.
    • Down past 000059 when Purchaser sits at ...0005: the down removes the memberships and the row (or refuses if a user would be stranded), and the up recreates and backfills, so the two stay symmetric.
    • Other flags: 000059's down drops system_managed for every group. Later migrations that set it (000096, 000125) re-run on the way back up.
  • Tests (scratch archive of the head SHA):
    • go build ./... and go vet -tags integration on config, api and database: clean.
    • go test -race -count=1 ./internal/config/ ./internal/api/ ./internal/mocks/: 5926 passed.
    • Under the heavy lock, go test -race -tags integration -count=1 ./internal/config/ ./internal/database/...: ok for config (62.9s), database, postgres/migrations (89.2s, which includes TestMigrations_DownToOneRoundTrip) and testhelpers.
  • Mutation probes (scratch copy; each restored and confirmed byte-identical with cmp):
    • M1, handler: treat ErrPurchaseAlreadyRevoked as a generic error. TestCallAzureReturn_FlipFindsRowRevoked/already_revoked FAILS. Killed.
    • M2, store: drop AND revoked_at IS NULL. TestFlipPurchaseRevocationInFlight_CompareAndSet FAILS ("round 0: a flip that lost to the mark must not re-flag"). Killed.
    • M3, 000059: drop the name = 'Purchaser' backfill guard. DownUpRoundTrip FAILS ("must not add an admin to Standard Users"). Killed.
    • M4, 000059: drop the ...0007 system_managed restore. DownUpRoundTrip FAILS on the group snapshot. Killed.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 85d688403cf1a3b4f9081816804e790f84b8d497

Blocking:

  • none

Non-blocking:

  • 000059_seed_purchaser_group.up.sql (round trip), separation of duties: now that a rollback below 000059 can be re-applied, the re-run of 000064 also re-runs its admin backfill. That backfill adds every Administrators member back into Purchaser (execute/approve-any/retry-any purchases), including admins who deliberately left Purchaser for separation of duties. The behavior is in 000064 and isn't changed here, but before this PR the re-apply stopped at 000059 and never reached it. Rolling back below 000059 in production is unrealistic, so I don't consider it blocking. It would be worth a line in the 000064 header or the runbook.

  • internal/config/store_postgres.go FlipPurchaseRevocationInFlight: the zero-row disambiguation runs a separate SELECT EXISTS outside the UPDATE. Suppose a row is deleted between the two statements. The result is "not found", and the handler logs it and continues to Return, exactly as on main, so this is not a regression. A single UPDATE ... RETURNING combined with a CTE over the row would make it one statement, but that's optional.

  • The PR body's out-of-scope notes are accurate: two first-time revokes can both pass the flip, and the flip keys on purchase_id alone. The first one, whether to make the flip a real claim, is an owner design question about the money path.

  • (Raised in an earlier review; I agree.) This is pre-existing: ClearRevocationInFlight (handler_purchases_revoke.go:905) is unconditional. A losing duplicate Return can therefore clear the flag while the winning request sits between its Return and MarkPurchaseRevoked. If that mark then exhausts its retries, the finalize sweep never sees the row.

Attacked:

  • Money path:
    • On ErrPurchaseAlreadyRevoked the handler returns before returnClient.Post, so no second Azure Return is posted. Every other flip error is still best-effort (log and continue), as on main.
    • The already_revoked response has the same shape as the up-front idempotency check, and MarkPurchaseRevoked isn't called again.
    • The CAS is AND revoked_at IS NULL. Under READ COMMITTED the row lock serializes it with MarkPurchaseRevoked, so a flip that waited re-evaluates against the committed row.
    • Grep finds one non-test caller of the flip and no other store implementation besides the mock.
  • Migration:
    • Traced 000057, 000059 and 000064 for a forward fresh DB, a re-apply with Standard Users at ...0005, and a re-apply with ...0005 free.
    • The guard still refuses a "Purchaser" at any id other than ...0005 and ...0007, so the bare ON CONFLICT DO NOTHING can only absorb those two cases.
    • Existing databases are unaffected, because 000059 is already applied there. Nothing in pre-commit or CI checks migration checksums.
  • Fail-before (each change restored afterwards):
    • Dropping AND revoked_at IS NULL fails TestFlipPurchaseRevocationInFlight_CompareAndSet, on both the revoked-row case and the race case.
    • Bypassing the ErrPurchaseAlreadyRevoked branch fails TestCallAzureReturn_FlipFindsRowRevoked/already_revoked.
    • Reverting 000059.up to main fails both DownUpRoundTrip subtests and DownToOneRoundTrip with the "already exists with a different id" refusal.
    • Mutation: dropping the backfill's name = 'Purchaser' fails with "must not add an admin to Standard Users".
  • Ran on 85d6884:
    • go build ./...; go vet ./... with and without -tags integration: clean.
    • go test -race -short -count=1 on api, config, migrations and mocks: ok.
    • go test -race -tags integration -count=1 on ./internal/config/ and ./internal/database/postgres/migrations/ (whole packages): ok.
    • -run 'Revoke|Revocation|RunNow' ./internal/api/ with the integration tag: ok.
    • golangci-lint v2.10.1 with and without the integration tag: 0 issues.
    • gocyclo -over 10 on the changed Go files: silent.

Body-stale:

  • (Raised in an earlier review; I agree.) "On a fresh database no users exist at 000059" holds only when app.admin_email is unset. 000005 and 000006 seed an admin when an operator sets it. The narrower backfill is a fix in that case, not a regression.

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.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at 85d688403cf1a3b4f9081816804e790f84b8d497 merged onto main (fe8b2136), together with #1513, #1514, #1516 in that order (combined tree f683b14a, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 3 approving independent review(s) on it. Nothing was run against any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 26, 2026 22:38
@IAmJSD
IAmJSD requested a review from cristim as a code owner October 26, 2026 22:38
@IAmJSD
IAmJSD merged commit 2faf67f into LeanerCloud:main Oct 26, 2026
41 checks passed
@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

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 85d68840 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 85d688403cf1a3b4f9081816804e790f84b8d497.

@cristim

cristim commented Oct 26, 2026

Copy link
Copy Markdown
Member

Independent review 2 (claude-fleet, cold): APPROVE 85d6884
Note: already MERGED. origin/main 2faf67f is the squash of #1516, and its diff is identical to the PR diff. No new migration (0 files added); highest on main is 000147.

Findings:

  • internal/config/store_postgres.go:2989-2994, internal/api/handler_purchases_revoke.go:206,790 - the CAS matches only revoked_at IS NULL, so it closes only the window after Mark. Two revokes that both load the row before either Mark both pass the guard at :206, and both flips return nil (proven: two concurrent flips on real postgres, both errors nil), so both post a Return. If Azure rejects the loser, handleAzureReturnError (:905) clears the winner's flag through ClearRevocationInFlight, which has no predicate (store_postgres.go:3017). If the winner's Mark then fails every retry, the row drops out of GetPurchaseHistoryInFlight (proven) and the UI offers revoke again. This is pre-existing and the PR does not make it worse. Follow-up: add AND revocation_in_flight = false and answer RECONCILE_PENDING when an unrevoked row is already in flight. [non-blocking]
  • 000059_seed_purchaser_group.up.sql:97 - the backfill condition also changes the forward path, not just a re-apply. Starting at version 58 with an admin, base ends with {Admins, Standard Users, Purchaser, RI Exchanger} and head ends with {Admins, Purchaser, RI Exchanger}. Fresh installs that seed an admin from app.admin_email will now differ from existing DBs by one redundant Standard Users membership. That membership is harmless, because admin:* covers every Standard Users verb, but the PR body does not mention the change. [non-blocking]
  • handler_purchases_revoke.go:836 - if the re-read fails, the already_revoked body carries revoked_at: "". [nit]
  • migrate_down_all_test.go:542 - the down-to-1 test runs only on an empty DB. With two admins, the down to 1 fails at the users_one_admin re-create. That limitation is pre-existing. [non-blocking]

Attacked:

  • Revoke flow: AWS and GCP return 422 at dispatch (:390-395), so only Azure reaches the flip. Already_revoked is the same type and status (200) as the up-front path at :194. Base instead posted a second Return. No new err text reaches the client, because flip errors are only logged. A failed Return still clears the flag (TestCallAzureReturn, existing guard tests).
  • Not-found vs revoked: UPDATE then EXISTS. No path un-revokes a row, so the re-read is stable. It is reached only after authorizeSessionRevoke and is never exposed as not-found, so it gives no oracle.
  • 000059: full row_to_json dumps of groups and users, diffed before and after latest->58->latest and latest->1->latest on real postgres. Groups are identical apart from updated_at and the jsonb order of Read-Only Users permissions. User diffs are covered under SECURITY. system_managed: an operator's change at ...0007 is already lost when the down drops the column. The up restores 000064's original TRUE, so the up adds no regression.
  • Checksums: golang-migrate keeps none, and the workflows (ci, database-migration, pre-commit) run no migration-immutability check.
  • Fail-before on base (head tests plus only the new error var): the CAS subtests "revoked row" and "racing a mark" FAIL, FlipFindsRowRevoked FAILS, both DownUpRoundTrip subtests and DownToOne FAIL with "migration 000059: a group named 'Purchaser' already exists", and ForeignPurchaserStillRefused passes (expected).
  • Probes, each restored and checked with cmp: M1 drop revoked_at IS NULL caught; M2 already-revoked mapped to not-found caught; M3 guard reverted caught; M4 backfill condition reverted caught (Standard Users subtest); M5 system_managed restore dropped caught; M6 back to ON CONFLICT (id) caught ("...0005 free"); M7 handler ignores the sentinel caught; M8 no re-read caught.

Verified locally:

  • go build ./... ok. go vet with and without -tags integration on api, config and migrations: clean. gofmt -l on the 8 changed .go files: empty.
  • go test -race on api and config: ok. -tags integration -race on config (73s) and migrations (88s): ok. -tags integration -run 'Revoke|Revocation|AzureReturn' ./internal/api/: ok. -v confirms the new tests RAN and did not skip.
  • golangci-lint 2.11.4: one prealloc in store_postgres_history_page_integration_test.go:141, a file this PR does not touch.
  • merge-tree onto the origin/main parent 2faf67f~1 is clean. The PR tests also pass on an export of the merged 2faf67f.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

IAmJSD added a commit to IAmJSD/cloud-commitments-platform that referenced this pull request Oct 26, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/bug Defect urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants