Skip to content

Conversation

@cathteng
Copy link
Member

@cathteng cathteng commented Dec 1, 2025

This is extremely noisy and doesn't add any value

@cathteng cathteng requested review from a team as code owners December 1, 2025 17:50
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 1, 2025
Comment on lines 189 to 190
continue

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Removing lifecycle.record_halt() for unfurlable links causes IntegrationEventLifecycle to incorrectly record them as SUCCESS instead of HALTED.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

When a link cannot be unfurled (i.e., link_type is None or args is None), the removal of lifecycle.record_halt() prevents explicit state setting. Due to the IntegrationEventLifecycle context's default assume_success=True, the event is incorrectly recorded as SUCCESS upon context exit instead of HALTED. This misrepresents the actual success rate of Slack integration's link unfurling, leading to corrupted metrics and misleading operational data.

💡 Suggested Fix

Either restore lifecycle.record_halt() and implement sampling/filtering to reduce noise, or initialize the IntegrationEventLifecycle context with assume_success=False to ensure unhandled exits default to HALTED.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/integrations/slack/webhooks/event.py#L189-L190

Potential issue: When a link cannot be unfurled (i.e., `link_type is None or args is
None`), the removal of `lifecycle.record_halt()` prevents explicit state setting. Due to
the `IntegrationEventLifecycle` context's default `assume_success=True`, the event is
incorrectly recorded as `SUCCESS` upon context exit instead of `HALTED`. This
misrepresents the actual success rate of Slack integration's link unfurling, leading to
corrupted metrics and misleading operational data.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 4620354

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's correct :)

@cathteng cathteng enabled auto-merge (squash) December 1, 2025 18:10
@cathteng cathteng merged commit 1928c2c into master Dec 1, 2025
67 checks passed
@cathteng cathteng deleted the cathy/eco/remove-halt-unfurl branch December 1, 2025 18:13
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.

3 participants