Skip to content

fix(desktop): return typed non-500 for expected Sentry poll states (#9139)#9145

Merged
kodjima33 merged 1 commit into
mainfrom
watchdog/issue-9139-sentry-poll-500
Jul 6, 2026
Merged

fix(desktop): return typed non-500 for expected Sentry poll states (#9139)#9145
kodjima33 merged 1 commit into
mainfrom
watchdog/issue-9139-sentry-poll-500

Conversation

@kodjima33

@kodjima33 kodjima33 commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Bug (#9139)

desktop-backend returns repeated 500s on POST /v1/webhooks/sentry/poll in prod (24x/2h), masking real failures.

Root cause

poll_sentry_feedback returns a blanket StatusCode::INTERNAL_SERVER_ERROR whenever the Sentry API call is non-2xx, the request fails, or the body doesn't parse. Prod logs show the actual state is a persistent 403 Forbidden from Sentry on every ~5-min poll (the SENTRY_AUTH_TOKEN lacks permission), so every poll emits a 500 even though nothing in this service is broken:

Sentry poll: fetching recent feedback issues
Sentry poll: found 122 existing sentry_feedback action items ...
Sentry poll: API returned 403 Forbidden        <- returned 500 to caller

Fix

Classify expected external states — auth expiry/permission (401/403), rate limit (429), upstream 5xx, network unreachable, and unparseable body — as a typed 2xx "skipped" response with a compact warn log instead of a 500. The success path is unchanged. Genuine server misconfiguration (missing SENTRY_ADMIN_UID/SENTRY_AUTH_TOKEN) still returns 500.

Response shape mirrors the success body so callers can treat ok/skipped uniformly:

{ "status": "skipped", "reason": "sentry_auth_error", "sentry_status": 403, "created": 0, "skipped": 0, "total_fetched": 0 }

Note (separate ops action)

The underlying 403 is an ops issue — SENTRY_AUTH_TOKEN for org mediar-n5 needs re-permissioning/rotation. This PR only stops the error-log noise; polling stays a no-op until the token is fixed.

Verification

  • cargo test — 2 new hermetic regression tests pass (sentry_poll_skip_reason_classifies_expected_states, sentry_poll_skipped_body_is_typed_non_500_shape)
  • cargo build clean, rustfmt applied

🤖 automated by hourly watchdog; opened for review, not merged.

Review in cubic

…9139)

/v1/webhooks/sentry/poll returned a blanket 500 for any Sentry API
non-2xx, network error, or parse failure. In prod Sentry returns 403
Forbidden on every ~5min poll (token lacks permission), so desktop-backend
logged persistent 500s (24x/2h) that mask real failures.

Classify expected external states (auth expiry/permission, rate limit,
upstream/network failure, unparseable body) as typed 2xx skipped
responses with compact warn logs instead of 500. Adds a hermetic
regression test for the status classification.

Note: the underlying 403 is an ops issue (SENTRY_AUTH_TOKEN needs
re-permissioning) tracked separately; this fix stops the error-log noise.

Verified: cargo test (2 new tests pass), cargo build clean, rustfmt applied.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No issues found across 1 file

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Reviewed the Sentry poll fix for #9139 — this looks clean and well-targeted.

What it does
Converts the blanket 500 returned by POST /v1/webhooks/sentry/poll on expected external states (auth/permission 401-403, rate-limit 429, upstream 5xx, network unreachable, unparseable body) into a typed 2xx {"status":"skipped", ...} response. Genuine server misconfiguration (missing SENTRY_ADMIN_UID / SENTRY_AUTH_TOKEN) still returns 500, which is the right call.

Strengths

  • The "skipped" body mirrors the success body shape (status/created/skipped/total_fetched), so callers can treat ok/skipped uniformly — good design.
  • Downgrades error!warn! for transient external states, which directly addresses the prod error-log spam without hiding real failures.
  • sentry_poll_skip_reason keeps the classification in one place and is covered by a focused regression test.
  • Single-file, +82/-12, coherent scope around one outcome.

Verification

  • Ran the two new hermetic unit tests (sentry_poll_skip_reason_classifies_expected_states, sentry_poll_skipped_body_is_typed_non_500_shape) in a clean Rust 1.88 build — both pass.
  • CI (rust, lint, Swift build) is green.

Minor note (non-blocking)
sentry_unreachable and sentry_bad_response are returned directly for network/parse failures and aren't routed through sentry_poll_skip_reason (which only maps HTTP status codes). That's consistent since those reasons don't correspond to an HTTP status, just flagging in case you want them consolidated later.

Net: solid fix for the log-noise issue. Holding off on formal approval per review process; a maintainer should be good to merge if they agree with the error-vs-skipped boundary. cc @kodjima33

@kodjima33 kodjima33 merged commit 1a1985e into main Jul 6, 2026
5 checks passed
@kodjima33 kodjima33 deleted the watchdog/issue-9139-sentry-poll-500 branch July 6, 2026 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants