fix(integrations): Ignore reordered and replayed inbound status webhooks - #121084
Open
vaind wants to merge 5 commits into
Open
fix(integrations): Ignore reordered and replayed inbound status webhooks#121084vaind wants to merge 5 commits into
vaind wants to merge 5 commits into
Conversation
Every provider converts an issue open/close event into a Sentry resolve/unresolve as a delta — GitHub and GitLab map the action verb, VSTS and Jira compare a from/to state pair — so a webhook delivered out of order writes an old status over a newer one. Delivery is not ordered: a failed delivery is retried with exponential backoff and lands behind events that were originally after it, and for providers in skip_on_failure_providers the drain skips a failed message outright. A close and reopen three seconds apart, delivered in reverse, leaves the group resolved with a GroupResolution and resolution notifications sent while the issue is open upstream. sync_status_inbound now compares the provider's own timestamp for the change against the newest event already processed for the same issue, held on a new nullable ExternalIssue.status_updated_at, and drops anything not strictly newer. Comparing provider time to provider time is what makes this safe: comparing against Sentry-side arrival time would suppress a legitimate follow-up whose provider timestamp precedes the previous event's apply time. The webhook handlers normalize their own timestamp into the task payload, so the shared task stays free of per-provider shapes. The guard is inert when either side is missing, which covers payloads enqueued before the key existed. The unresolve path, which had no guard at all, is additionally narrowed to the groups the event actually changes, so issue_unresolved no longer fans out for groups that were already unresolved. Refs #121057, #121059
Contributor
|
This PR has a migration; here is the generated SQL for for --
-- Add field status_updated_at to externalissue
--
ALTER TABLE "sentry_externalissue" ADD COLUMN "status_updated_at" timestamp with time zone NULL; |
Keep the why — provider-clock comparison, inert on a missing timestamp — and drop the restatement.
121059 took 1150 after its own renumber, and it is further along. The dependency still points at 1149 and will need repointing once 1150 lands.
Contributor
|
This PR has a migration; here is the generated SQL for for --
-- Add field status_updated_at to externalissue
--
ALTER TABLE "sentry_externalissue" ADD COLUMN "status_updated_at" timestamp with time zone NULL; |
A bare `status_updated_at` reads as when Sentry updated the status; the column holds the provider's clock. `provider_` rather than `scm_` because ExternalIssue spans Jira and VSTS.
Contributor
|
This PR has a migration; here is the generated SQL for for --
-- Add field provider_status_updated_at to externalissue
--
ALTER TABLE "sentry_externalissue" ADD COLUMN "provider_status_updated_at" timestamp with time zone NULL; |
vaind
marked this pull request as ready for review
August 4, 2026 22:06
vaind
added a commit
that referenced
this pull request
Aug 5, 2026
#121059) 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.
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.
Inbound issue status sync turns a provider's issue open/close event into a Sentry resolve/unresolve as a delta — GitHub and GitLab map the action verb, VSTS and Jira compare a from/to state pair — and
sync_status_inboundapplies whatever it is handed. Webhook delivery is not ordered, so an old delta gets applied on top of a newer one.This is an active production bug for GitHub, not a hypothetical.
githubis the default value ofhybridcloud.webhookpayload.skip_on_failure_providers, so a failed delivery in an issues mailbox is skipped and the rest drains past it. A user who closes an issue and reopens it three seconds later can have the pair land in reverse:The group is now resolved while the GitHub issue is open —
GroupResolutionwritten,issue_resolvedfired so resolution notifications go out, issue dropped from the unresolved stream, and nothing reconciles it afterwards. Symmetrically, a stale replay can reopen an issue a human just resolved in Sentry.The fix
Each webhook handler normalizes its provider's own timestamp for the change into the task payload (
issue.updated_at,object_attributes.updated_at,System.ChangedDate,issue.fields.updated).sync_status_inboundcompares it against the newest event already processed for that issue and drops anything not strictly newer. The watermark is a new nullableExternalIssue.provider_status_updated_at— migration1151_externalissue_provider_status_updated_at, additive, no backfill. Theprovider_prefix carries the provenance in the name: a barestatus_updated_atwould read as when Sentry updated the status. #121059 usesscm_onPullRequestsince that model is SCM-only;provider_here becauseExternalIssuealso spans Jira and VSTS.Provider time on both sides is the design decision worth questioning. Comparing against Sentry-side arrival time instead (
date_added, activity timestamps) cannot work: delivery latency is seconds, the same scale as rapid user actions, so a legitimate reopen whose provider timestamp precedes the previous event's apply time would be suppressed — a worse failure than the bug. Provider-to-provider comparison is immune to how long delivery took. Reading payload state (issue.state) instead of the action verb does not help either: the stale close payload also saysstate: "closed", because that is what it was when the provider generated it.On the comparison operator. This guard treats equal timestamps as stale (
event_time <= last_event_time), while the sibling guards in #121059 and #121157 treat equal as fresh (<). That divergence is deliberate. This path consumes a delta — an action verb, or achangelog.from/.topair — and re-applying a delta on top of an intervening human action is destructive, so a redelivery at the same timestamp has to be dropped. Those two consume snapshots, which are idempotent to re-apply, so letting the later delivery win costs nothing.A missing timestamp makes the guard inert, so payloads enqueued before this key existed keep syncing as they do today. The unresolve path, which had no guard at all, additionally now narrows to the groups the event actually changes so
issue_unresolvedstops firing for groups that were already unresolved.Rollout
Prerequisite for #121057, which widens
skip_on_failure_providers— GitLab and VSTS must not be added until this lands. #121059 is the pull-request-side equivalent; the two are deliberately consistent in deriving order from provider time rather than arrival time.Migration numbering: #121059 renumbered to
1150_pullrequest_updated_atafter master landed1149, so this moved to1151to sit behind it rather than collide. The dependency here still points at1149, because1150is not on master yet — once it lands,./bin/update-migrationrepoints it. If master lands another migration before either merges, the lockfile will conflict again and the same command resolves it.The red
migration driftcheck is pre-existing, not this PR.tools/migrations/squash.pydeletes every migration of each app whose lockfile head is not0001_squashed_*(always includingsentry), but_cleared_depsonly rewrites cross-app dependencies inside each already-squashed app's0001_squashed_*.py.discoverandexploreare squashed while their pre-squash originals are still checked in, so those leftovers keep pointing at deleted nodes and Django's graph validation fails beforemakemigrationsruns. Runningsquash.pyon a cleanorigin/masterwith no added migration reproduces it (discover.0002 → explore.0006, versusdiscover.0001 → sentry.0945here — same failure, whichever dangling edge Django hits first). The workflow has failed on all of its last 100 runs since 2026-07-20, across dozens of unrelated branches.check migration,backend migration tests, andcheck if any migration changesare green here.