Repository navigation
feat(jira): Add a cell RPC to apply Forge issue-updated events - #130682
roggenkemper wants to merge 5 commits into
Conversation
jira_forge_service.handle_issue_updated applies a Forge avi:jira:updated:issue event to the linked Sentry issues in a cell, using the same handlers as the Connect issue-updated webhook. Control will verify events and call it through an outbox; that caller lands separately, after this method has deployed to every cell. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8c27723. Configure here.
Run the move, assignee and status handlers in their own try blocks so one failure, such as a Jira error during the assignee email lookup, no longer drops the event's other changes. Test the service in every silo mode and run a move event shaped like Forge's through the real handlers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| # RPC services register when their module is imported, and this package is loaded at | ||
| # startup in every silo, so cells can serve jira_forge before control calls it. | ||
| from sentry.integrations.services.jira_forge import jira_forge_service # noqa: F401 |
There was a problem hiding this comment.
uh idt we need the comment? But also this way of importing seems a little 💫 , why don't other HC services import like this ?
There was a problem hiding this comment.
Other services get registered because a caller in each silo imports them, and normally the caller ships in the same PR. With the method-before-caller rule, nothing in the cell imports this service until the next PR, so cells couldn't serve it (verified on a cell-mode startup: not registered without this import). Trimmed the comment to one line.
| # Same order as the Connect webhook: rekey first, since the other handlers look | ||
| # the issue up by its new key. | ||
| handlers: list[tuple[str, Callable[[Any, Mapping[str, Any]], None]]] = [ | ||
| ("issue_moved", handle_issue_moved), | ||
| ( | ||
| "assignee_change", | ||
| partial(handle_assignee_change, use_email_scope=settings.JIRA_USE_EMAIL_SCOPE), | ||
| ), | ||
| ("status_change", handle_status_change), | ||
| ] |
There was a problem hiding this comment.
why this list of handlers? seems a bit of an odd change from the prev 1 by 1 impl. Is it because of the exception handling? I think we could prob do something like _handle_exception and then get the .name of the handler instead?
Also also the partial is kinda weird to me, feels a bit hacky?
There was a problem hiding this comment.
Agreed, swapped it for a small _apply_forge_change(handler, ...) helper that logs with handler.__name__, and the handlers are called one by one again.
| @@ -0,0 +1,126 @@ | |||
| from typing import Any | |||
There was a problem hiding this comment.
le bot flag
- Missing tests/.../services/jira_forge/init.py (high).
- The neighbouring repository/ folder has no init.py either, and both folders contain a test_impl.py.
- With pytest's default import mode, both files load as the module test_impl. Any run that collects both fails with "import file mismatch".
- The PR's CI passed only because it ran selected tests on one shard. fix(tests): Add missing __init__.py files to test packages #130061 fixed the same failure five days ago.
- Fix: add an empty init.py.
| # event for this integration. A failure is reported and only that change is | ||
| # dropped, so one handler can't cost the event its other changes. | ||
| try: | ||
| handler(integration, event) |
There was a problem hiding this comment.
le bot flag
- A failed rekey loses the whole event, and swallowing every exception drops temporary failures for good (medium-high).
- If handle_issue_moved raises, the ExternalIssue row stays at the old key. The assignee and status handlers look up the new key, match nothing, and return
quietly.- Suggestion: re-raise if the move fails, or re-raise the temporary error types. Another option is to have the RPC queue a cell task with bounded retries
There was a problem hiding this comment.
hmm it is a bit odd that these aren't separate tasks that chain eachother or maybe its own task since if handle_issue failing is that bad then we should have some retries on that part. I see the current impl for connect doesn't have a separate task so 🤷♀️ there but I guess we should have some retries
There was a problem hiding this comment.
Moved the work into a cell task: the RPC now just queues apply_forge_issue_event, which retries temporary failures (Jira 5xx/timeouts/rate limits, RPC errors) up to 5 times. The handlers are idempotent, so re-running the event is safe. Non-temporary errors (e.g. a 404 for a deleted account) are reported and only that change is skipped. Also reordered to move, then status, then assignee, since the assignee email lookup is the only Jira call, so it can't hold up the status update.
The RPC now queues apply_forge_issue_event instead of applying the event inline. Jira API errors raise so the task retries the whole event, which the handlers tolerate. Other errors are reported and only that change is skipped. Also add the missing test package __init__.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ents Apply status before assignee, whose email lookup is the only Jira call, so a failure there can't hold up the status change. Retry only temporary failures (5xx, timeouts, rate limits, host and RPC errors); report and skip the rest. Give the task a 60s processing deadline, longer than the Jira client's timeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Treat connection resets, and API errors with no HTTP status (Jira never responded), as temporary so the task retries them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adds
jira_forge_service.handle_issue_updated, a cell RPC that queues a Forgeavi:jira:updated:issueevent to be applied to the linked Sentry issues in that cell. The new cell taskapply_forge_issue_eventruns the same handlers as the Connect issue-updated webhook: move/rekey first (the others look the issue up by its new key), then status, then assignee, whose email lookup is the only call to Jira.Control will verify Forge events and hand them to cells through a control outbox that calls this method; the cell does no authentication of its own and never sees the Forge tokens. That caller lands in a separate PR once this method has deployed to every cell, per the RPC-before-caller rule. Nothing calls it yet.
The RPC queues a task instead of applying the event inline, so a temporary failure is retried instead of blocking the control outbox, which retries without limit. Temporary failures (Jira 5xx, timeouts, rate limits, host and RPC errors) are retried up to 5 times; the handlers are idempotent, so re-running the event is safe. Anything else, such as a 404 for a deleted Jira account, is reported and only that change is skipped.
sentry.integrations.jiraimports the service so it registers at startup in every silo: RPC services only register when their module is imported, and without that import a cell wouldn't serve the method until the caller's PR shipped too.The Forge event shapes were checked against real events from a test site: a move arrives as a
projectitem plus aKeyitem withfromString/toStringset, whichhandle_issue_movedreads as-is. The tests cover that move event through the real handlers, applied twice.