Skip to content

fix(integrations): Ignore reordered and replayed inbound status webhooks - #121084

Open
vaind wants to merge 5 commits into
masterfrom
fix/inbound-status-sync-ordering
Open

fix(integrations): Ignore reordered and replayed inbound status webhooks#121084
vaind wants to merge 5 commits into
masterfrom
fix/inbound-status-sync-ordering

Conversation

@vaind

@vaind vaind commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

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_inbound applies 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. github is the default value of hybridcloud.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:

T+0:00  closed    → transient failure, rescheduled with backoff
T+0:03  reopened  → delivered, group left unresolved
T+4:00  closed    → retry lands, group RESOLVED

The group is now resolved while the GitHub issue is open — GroupResolution written, issue_resolved fired 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_inbound compares it against the newest event already processed for that issue and drops anything not strictly newer. The watermark is a new nullable ExternalIssue.provider_status_updated_at — migration 1151_externalissue_provider_status_updated_at, additive, no backfill. The provider_ prefix carries the provenance in the name: a bare status_updated_at would read as when Sentry updated the status. #121059 uses scm_ on PullRequest since that model is SCM-only; provider_ here because ExternalIssue also 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 says state: "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 a changelog.from/.to pair — 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_unresolved stops 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_at after master landed 1149, so this moved to 1151 to sit behind it rather than collide. The dependency here still points at 1149, because 1150 is not on master yet — once it lands, ./bin/update-migration repoints it. If master lands another migration before either merges, the lockfile will conflict again and the same command resolves it.

The red migration drift check is pre-existing, not this PR. tools/migrations/squash.py deletes every migration of each app whose lockfile head is not 0001_squashed_* (always including sentry), but _cleared_deps only rewrites cross-app dependencies inside each already-squashed app's 0001_squashed_*.py. discover and explore are squashed while their pre-squash originals are still checked in, so those leftovers keep pointing at deleted nodes and Django's graph validation fails before makemigrations runs. Running squash.py on a clean origin/master with no added migration reproduces it (discover.0002 → explore.0006, versus discover.0001 → sentry.0945 here — 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, and check if any migration changes are green here.

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
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 3, 2026
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

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

for 1150_externalissue_status_updated_at in sentry

--
-- Add field status_updated_at to externalissue
--
ALTER TABLE "sentry_externalissue" ADD COLUMN "status_updated_at" timestamp with time zone NULL;

vaind added 3 commits August 4, 2026 09:40
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.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

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

for 1151_externalissue_status_updated_at in sentry

--
-- 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.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

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

for 1151_externalissue_provider_status_updated_at in sentry

--
-- Add field provider_status_updated_at to externalissue
--
ALTER TABLE "sentry_externalissue" ADD COLUMN "provider_status_updated_at" timestamp with time zone NULL;

@vaind
vaind marked this pull request as ready for review August 4, 2026 22:06
@vaind
vaind requested review from a team as code owners August 4, 2026 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant