fix(scm): Make PR lifecycle writes monotonic across reordered webhooks

[vaind]

SCM webhooks arrive out of order, and every pull-request / merge-request payload is a full snapshot of the PR — so an older delivery silently overwrites a newer one. Both the GitHub and GitLab handlers wrote theirs through an unguarded update_or_create.

T+0:00  synchronize     → transient cell 500, rescheduled ~4 min out
T+1:00  closed (merged) → delivered: state=merged, merged_at, closed_at set
T+4:00  synchronize     → retry lands: state=open, merged_at=None

The PR is then shown as open in Sentry while it is merged upstream, permanently. The same write also rolls back closed_at, so run_deferred_emission reads the PR as reopened and cancels its metrics emission; flips open ↔ non-open, so pull_request_state_changing writes a spurious "reopened this pull request" onto every issue the PR resolves; and re-derives group links from the stale body, deleting the GroupLink.

Two independent sources of reordering, both addressed here: retry backoff (schedule_next_attempt — and for skip_on_failure_providers the drain skips the failed message outright), and concurrency — _run_parallel_delivery_batch hands the next hybridcloud.webhookpayload.worker_threads payloads from one mailbox to a single threadpool (16 in production), and GitHub buckets every pull_request event for a repo into one mailbox.

The fix

Staleness is decided on the provider's own last-modified time, stored in a new nullable PullRequest.provider_updated_at — migration 1150_pullrequest_provider_updated_at, additive and nullable, no backfill. date_added/date_updated are arrival time and can't order anything, and no existing column carried provider time. The provider_ prefix marks that the column holds someone else's clock: bare updated_at is the usual ORM convention for a row-modification timestamp, so it would read as Sentry-local time. The same prefix is used by the sibling guards on ExternalIssue (provider_status_updated_at in #121084, provider_assignee_updated_at in #121157) — those three are read together, so a naming divergence between them would look meaningful when it isn't.

A stale snapshot is dropped whole rather than field by field: every mutable column comes from the same point in time, and a state-only guard would still fabricate the timeline entries and delete the issue links listed above. A second rule — merged is terminal — covers what timestamps can't: rows predating the column (no backfill, so all of them at first) and GitHub's one-second resolution. It isn't sufficient alone, because closed → open is a real transition that only provider time separates from a replay. Equal timestamps are not stale.

Writers are serialized across the whole decision, otherwise two concurrent deliveries for one PR both read the pre-write row and the older write lands last. That takes two locks, because a row lock alone can't cover the case where the row doesn't exist yet: SELECT ... FOR UPDATE grants no exclusion on a first delivery, so both writers would skip the guard and Django's update_or_create would apply the loser's defaults over the winner's insert. A pg_advisory_xact_lock keyed on (repository_id, key) — the unique constraint that arbitrates that race — is keyed on a value rather than a tuple, so it exists before the row does; it is transaction-scoped, so it releases on commit or rollback, cannot expire mid-write, and leaves no session state for a pooler to lose. select_for_update stays as a second line of defence for writers that create the row by another path (the pr_metrics stub). The advisory lock is always taken first and by this path alone, and every other writer of the row takes only the row lock and never waits on it, so no cycle can form.

pr_metrics.handle_metrics writes PullRequestMetrics from the same payload and is held to the same verdict. Without that, select_verdict (src/sentry/pr_metrics/emit.py:175-178) reads zeroed discussion counts off a PR that had real reviewer engagement and emits a CLOSED_UNMERGED that _claim_terminal_event makes permanent.

Skips report scm.webhook.pull_request.stale_snapshot (tagged by provider) and pr_metrics.metrics.stale_snapshot.

Landing order

Prerequisite for #121057, which widens skip_on_failure_providers to github_enterprise, bitbucket, and bitbucket_server. GitHub Enterprise inherits this guard by subclassing the GitHub handler.

Delivery and retry semantics in deliver_webhooks.py are untouched.

[github-actions]

This PR has a migration; here is the generated SQL for src/sentry/migrations/1149_pullrequest_updated_at.py

for 1149_pullrequest_updated_at in sentry

--
-- Add field updated_at to pullrequest
--
ALTER TABLE "sentry_pull_request" ADD COLUMN "updated_at" timestamp with time zone NULL;

[github-actions]

This PR has a migration; here is the generated SQL for src/sentry/migrations/1150_pullrequest_updated_at.py

for 1150_pullrequest_updated_at in sentry

--
-- Add field updated_at to pullrequest
--
ALTER TABLE "sentry_pull_request" ADD COLUMN "updated_at" timestamp with time zone NULL;

[github-actions]

This PR has a migration; here is the generated SQL for src/sentry/migrations/1150_pullrequest_scm_updated_at.py

for 1150_pullrequest_scm_updated_at in sentry

--
-- Add field scm_updated_at to pullrequest
--
ALTER TABLE "sentry_pull_request" ADD COLUMN "scm_updated_at" timestamp with time zone NULL;

[github-actions]

This PR has a migration; here is the generated SQL for src/sentry/migrations/1150_pullrequest_provider_updated_at.py

for 1150_pullrequest_provider_updated_at in sentry

--
-- Add field provider_updated_at to pullrequest
--
ALTER TABLE "sentry_pull_request" ADD COLUMN "provider_updated_at" timestamp with time zone NULL;

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit ac4a7ad. Configure here.}

[giovanni-guidini]

makes sense to me... that's the only place we create PullRequest objects?

[giovanni-guidini]

I thought GitHub in particular had a reopened action so we could in theory guard on closed -> open transition (assuming the valid one is closed -> reopened.

Not sure how worth it that it.
Not sure if other providers support it.

[vaind]

tldr; it's not a clear cut because the same status is caried on other events we shouldn't skip

Claude's version:

The premise checks out on both providers — GitHub sends reopened, and the GitLab MR handler already branches on action in ("reopen", "open") (gitlab/webhooks.py:519). So the rule is writable for both. I went and worked through whether it holds up, and I don't think it can replace the timestamp — but the reasoning isn't obvious, so it's worth writing down rather than just asserting.

The rule would have to be "stored is closed + payload says open ⇒ require action: reopened." reopened isn't the only action carrying state: open — after a reopen, so does every one of the ~17 actions in _ACTIVITY_ACTIONS, plus edited. So it can only fire on the transition itself.

Where it breaks: it's correct only if we're guaranteed to have processed the reopened event, and we aren't. The comment on these columns already concedes "a late-installed integration, a missed/dropped webhook". If a reopened is dropped, the next synchronize arrives with state: open under a non-reopen action and gets rejected — and since a rejected write doesn't advance provider_updated_at, so does the one after it, and the one after that. The PR is stuck closed permanently and silently. The timestamp gets this case right: that synchronize really is newer than the close, so it's applied.

That asymmetry is what decided it for me. The action tells you what triggered this event, not whether you saw everything before it — and lossy delivery is precisely the condition the guard exists for.

It's also narrower than it looks. The action only speaks to lifecycle transitions. Two reordered synchronize payloads carry the same state and the same action, differing only in head_sha / title / message; the timestamp orders them, the action can't. So at best it complements.

The one place it would genuinely add something, since this is the strongest case for it: the timestamp guard is inert while provider_updated_at is NULL, which is every row until its first write after the migration. But that window is smaller than it first appears — the first guarded write sets the mark from its own payload whether or not the guard fired, so a reordered pair comes out right even starting from NULL (older first → applied, mark = T1, newer applied; newer first → applied, mark = T2, older rejected). What's actually unguarded is a row whose state was set without setting the mark: a pre-migration closed row receiving a replay that straddles the deploy. Merged rows are covered by the terminal-merged rule, and stub rows have state = NULL so there's nothing to regress. Retry backoff is bounded, so this is PRs closed shortly before the deploy with an in-flight retry, and it closes permanently once each PR takes one webhook.

So the trade is a narrow, self-closing, deploy-boundary window against a permanent stuck-closed failure mode. I don't think that's worth it. Confining the action rule to just the NULL window doesn't rescue it either — a rejection leaves the mark NULL, so the row stays in the fallback regime and keeps rejecting.

I've updated the docstring on this line so the next reader doesn't have to re-derive it:

The event action can't stand in there: it says what triggered this event, not whether the reopened before it was delivered, so requiring that action would permanently reject every later payload on a PR whose reopen we missed.

Happy to be argued out of this. If you'd want visibility rather than enforcement, the cheap version is a counter for when a NULL-mark row takes an open-state payload onto a closed row under a non-reopen action, which would measure whether that deploy window ever actually bites, with no behavioural risk. I lean against even that — it's instrumentation for something that closes on its own — but I'll add it if you'd rather have the data.

[vaind]

that's the only place we create PullRequest objects?

yes