Re-check review state inside the downgrade apply transaction - #2376
Merged
jakebromberg merged 3 commits intoSep 6, 2026
Merged
Conversation
The plan phase reads `self_signup_reviewed_at IS NULL AND self_signup_downgraded_at IS NULL` once, and the SES round trip then sits between that read and the write, so a review landing during the notify window was still followed by a downgrade — and because the marker stamp is terminal and the account leaves the digest cohort once reviewed, nothing ever reported the incorrect flip. `applyDowngrades` now re-selects the `auth_user` row inside its transaction and aborts both the role flip and the marker stamp when either column is non-NULL, so a manager review or a competing run of this same job that lands mid-window wins instead of losing. An aborted flip joins the existing `applied.raced` accounting — a warning, not a failure, and the run still exits 0 — with the `downgrade_raced` log line reworded to cover the broadened cause. Kill switch, on-air guard, and the plan and digest semantics are untouched; only the apply step changes.
…race guard is atomic
The in-transaction re-check added for BS#2373 was a bare SELECT, and `db.transaction()` runs at READ COMMITTED, so it was check-then-act rather than an atomic check-and-act: a review committing between the re-select and the `auth_member` UPDATE is invisible to both of the writes that follow, because the UPDATE's own guard is `role = 'dj'` and the marker stamp's guard is `self_signup_downgraded_at IS NULL`, and a review touches neither column. The window was narrower than trusting the plan-phase snapshot, but it still landed on the reviewed-AND-downgraded terminal state the re-check exists to prevent.
Appending `.for('update')` to the re-select closes it. A concurrent writer of `self_signup_reviewed_at` either waits on the row lock or holds it first, and when it holds it first READ COMMITTED's EvalPlanQual recheck re-applies the `IS NULL` predicates against the newly committed row version once the lock is granted -- the row filters out, and the transaction aborts into `raced` instead of flipping the role.
The docstring now records the lock order this creates: `auth_user` first, then `auth_member`. The BS#2362 approve endpoint will be the first real writer of `self_signup_reviewed_at` and touches both tables, so it has to take them in the same order or the two deadlock.
Adds a discriminating integration case: the marker stamped but the member role re-promoted back to `dj` -- production-reachable when a concurrent run downgrades and stamps, a manager re-promotes, and this run's stale apply lands. The `WHERE role = 'dj'` guard alone cannot tell that apart from a fresh downgrade and would flip a second time; the re-check aborts, leaving the re-promotion and the original marker instant intact. The unit abort test now asserts the strong form (`mockUpdate` never called at all) that its sibling review-landed test already used, and the mocked select chain exposes `.for()` off `.limit()` so both shapes resolve to the same queued result.
…dlock bound
The unit shim's .for() is now a jest.fn and the re-check spec asserts it was called with 'update' -- without this, deleting the lock (or weakening it to for('share'), which does not conflict with a reviewer's row update) would reintroduce the plan->apply race with a fully green suite, since the integration test is single-session and discriminates only the predicate re-check.
The lock-order docstring now records what bounds the wait when a future both-table writer takes the locks in the wrong order: the connection-level statement_timeout (5s default, no override in this job) throws, landing the account in failed with a non-zero exit for retry next run, rather than silently dropping into raced.
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
1 participant
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.
Closes #2373
What
applyDowngradesre-checked onlymember.role = 'dj'inside its transaction. Neitherauth_user.self_signup_reviewed_atnorauth_user.self_signup_downgraded_atwas re-evaluated, so the plan-phase snapshot — taken before the SES round trip — was trusted for the write. A review landing during that notify window was still followed by a downgrade, and the marker stamp made it terminal: the account leaves the digest cohort once reviewed, so nothing ever surfaced the incorrect flip. #2362 makes the approve endpoint the first real writer ofself_signup_reviewed_at, which is why this lands first.How
The apply transaction now re-selects the
auth_userrowFOR UPDATEand aborts the role flip and the marker stamp when either column is non-NULL, before touching anything. The row lock is what closes the race rather than just narrowing it:db.transaction()runs at READ COMMITTED, so a bare re-select would still be check-then-act — a review committing between the re-select and theauth_memberUPDATE is invisible to both writes, since the UPDATE's guard isrole = 'dj'and the marker stamp's isself_signup_downgraded_at IS NULL, and a review touches neither. WithFOR UPDATE, a concurrent writer ofself_signup_reviewed_ateither waits on the lock or holds it first, and READ COMMITTED's EvalPlanQual recheck re-applies theIS NULLpredicates against the newly committed row version once the lock is granted, so the row filters out and the transaction aborts. A re-select rather than folding the columns into the guarded UPDATE's predicate, because those columns live onauth_userand the role flip writesauth_member— a different table. The docstring records the resulting lock order (auth_userfirst, thenauth_member) so #2362's approve endpoint, which will touch both, adopts the same order and does not deadlock against this job. An aborted flip reports through the existingapplied.racedaccounting: a warning, not a failure, and the run still exits 0. Thedowngrade_racedlog line is reworded, since "matched no auth_member row" is no longer the only cause. Kill switch, on-air guard, and the plan and digest semantics are unchanged — only the apply step moves.Tests
Unit: the re-check clause is pinned by shape (both
isNulls against theauth_usertable object), and both abort paths — review landed, marker stamped by a competing run — assert no role flip, no marker, and aracedoutcome. Every pre-existing apply test now steers the re-check to "still pending" first, so the happy-path invariants still read as before.Integration (real Postgres): a review stamped between plan and apply produces no flip and no marker; a concurrent run's role flip plus marker produces no double work and no overwrite of that run's marker instant; and the discriminating case — marker stamped but the account re-promoted back to
dj, where theWHERE role = 'dj'guard alone would happily flip a second time — aborts asracedwith the re-promotion and the original marker instant both intact. All assert theauth_user.rolesentinel stays untouched — the invariant this whole epic turns on.