Add token circuit breaker notification (issue #18886) #18954

geetxnshgoyal · 2025-12-07T09:56:49Z

When a Token::Workflow fails to deliver to the SCM with authorization errors ('Unauthorized request' or 'Request forbidden'), the token gets disabled. This change adds:

Event::TokenDisabled: New event triggered when token is disabled
Notification system: Sends notifications to token executor and members
Default subscriptions: Enabled by default for instant_email and web channels
Tests: Added comprehensive test coverage for the new functionality

The notification helps users immediately understand why their token was disabled and take corrective action.

Fixes #18886

Copilot

Pull request overview

This PR implements a notification system for when SCM/CI workflow tokens are automatically disabled due to authorization failures (unauthorized or forbidden responses from GitHub/GitLab). When a token is disabled, the system now creates an Event::TokenDisabled event that notifies the token executor and members about the issue.

Key Changes:

New Event::TokenDisabled event class with notification support for token executors and members
Enhanced WorkflowRun#update_as_failed to create notification events when disabling tokens
Comprehensive test coverage for the new event and notification creation

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`src/api/app/models/event/token_disabled.rb`	New event class defining the token disabled notification with receiver roles, subject generation, and helper methods
`src/api/app/models/workflow_run.rb`	Updated to create `Event::TokenDisabled` when disabling tokens due to authorization errors
`src/api/app/models/event/base.rb`	Registered `Event::TokenDisabled` in the notification_events list
`src/api/spec/models/event/token_disabled_spec.rb`	Test coverage for event methods including token_executors and subject generation
`src/api/spec/models/workflow_run_spec.rb`	Tests verifying event creation when tokens are disabled for forbidden/unauthorized responses
`src/api/db/data/20251207091632_create_default_token_disabled_subscriptions.rb`	Data migration for creating default event subscriptions (currently empty)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-07T10:01:43Z

src/api/app/models/event/token_disabled.rb

+    # Example of subject:
+    #   GitHub workflow token disabled
+    def subject
+      vendor = payload['scm_vendor']&.capitalize


The capitalize method will produce incorrect brand names: 'github' → 'Github' and 'gitlab' → 'Gitlab', but the correct brand names are 'GitHub' and 'GitLab' (with capital letters in the middle). Consider using a mapping like: {'github' => 'GitHub', 'gitlab' => 'GitLab'}[payload['scm_vendor']] or a case statement to ensure proper capitalization.

Suggested change

vendor = payload['scm_vendor']&.capitalize

vendor_map = {

'github' => 'GitHub',

'gitlab' => 'GitLab'

}

raw_vendor = payload['scm_vendor']

vendor = vendor_map[raw_vendor] || raw_vendor&.capitalize

@geetxnshgoyal nice to have, but I would also be fine as it is

@krauselukas thanks! I've already implemented the vendor mapping in the latest commit, so it now correctly displays GitHub, GitLab, and Gitea Tests updated too!

Copilot · 2025-12-07T10:01:43Z

src/api/app/models/event/token_disabled.rb

+    end
+
+    def event_object
+      Token.find(payload['token_id'])


The event_object method uses Token.find which will raise an ActiveRecord::RecordNotFound exception if the token has been deleted. This is inconsistent with the safer token private method (line 33) which uses find_by and returns nil if not found. Consider using Token.find_by(id: payload['token_id']) to handle the case where the token no longer exists gracefully.

Suggested change

Token.find(payload['token_id'])

Token.find_by(id: payload['token_id'])

Copilot · 2025-12-07T10:01:44Z

src/api/db/data/20251207091632_create_default_token_disabled_subscriptions.rb

+  def up; end
+


This data migration is empty - the up method has no implementation. Based on the PR description mentioning "Default subscriptions: Enabled by default for instant_email and web channels", this migration should create default EventSubscription records for the Event::TokenDisabled event type. See similar migrations like 20200424080753_generate_web_notifications.rb or 20250404105011_change_decision_report_subscriptions.rb for examples of how to create default subscriptions.

Suggested change

def up; end

def up

# Find the event type for TokenDisabled

event_type = EventType.find_by(name: 'TokenDisabled')

return unless event_type

# For each user, create default subscriptions for instant_email and web channels if not already present

User.find_each do |user|

%w[instant_email web].each do |channel|

EventSubscription.find_or_create_by!(

user_id: user.id,

event_type_id: event_type.id,

channel: channel

)

end

end

end

@geetxnshgoyal yes, that empty migration needs to be filled...

@krauselukas done! I've filled the migration using the standard [find_or_create_by!] pattern with proper receiver roles and channels It creates default subscriptions for both token_executor and token_member roles across instant_email and web channels all enabled by default.

krauselukas

The model for the notification type "NotificationToken" is missing, without it its not possible to render or create this type of notification. I would suggest you start implementing it, and then we can have another round of review :) Its also necessary to have a look on the notification controller and view components. We have filters for the individual notification types. Those would also be required in order to make this feature complete. Let us know if anything is unclear :)

krauselukas · 2025-12-08T17:18:39Z

src/api/app/models/event/token_disabled.rb

+    end
+
+    def parameters_for_notification
+      super.merge(notifiable_type: 'Token::Workflow', notifiable_id: payload['token_id'], type: 'NotificationToken')


@geetxnshgoyal here you add the parameters for the type: 'NotificationToken, but there is no corresponding model for this notification type. You need to add a model for it, similar to https://github.com/openSUSE/open-build-service/blob/master/src/api/app/models/notification_workflow_run.rb

@krauselukas thanks for telling that! I've now added the NotificationToken model in [notification_token.rb] with all the required methods also i filled the data migration to create the default subscriptions the model should now handle notification rendering properly!!

krauselukas · 2025-12-08T17:20:06Z

src/api/db/data/20251207091632_create_default_token_disabled_subscriptions.rb

+  def up; end
+


@geetxnshgoyal yes, that empty migration needs to be filled...

krauselukas · 2025-12-08T17:22:19Z

src/api/app/models/event/token_disabled.rb

+    # Example of subject:
+    #   GitHub workflow token disabled
+    def subject
+      vendor = payload['scm_vendor']&.capitalize


@geetxnshgoyal nice to have, but I would also be fine as it is

geetxnshgoyal · 2025-12-10T04:37:35Z

@krauselukas thanks for the thorough review! 🙌

Good news - I've already implemented the NotificationToken model in the latest push! You can find it at [notification_token.rb]
Regarding the notification controller and view components I can see that's important for completing the feature. Should I start working on those next, or would you prefer to review the current changes first? Happy to tackle them either way!

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-10T05:08:09Z

src/api/app/models/notification_token.rb

@@ -0,0 +1,58 @@
+class NotificationToken < Notification
+  def description
+    "Token #{notifiable.description.presence || 'Token'} was disabled"


The fallback text in the description method will result in redundant wording. When notifiable.description.presence is nil or empty, the description will be "Token Token was disabled" (the word "Token" appears twice).

Consider using an empty string as the fallback:

"Token #{notifiable.description.presence || ''} was disabled".strip

Or restructure to avoid redundancy:

"Token #{notifiable.description.presence || '(no description)'} was disabled"

Suggested change

"Token #{notifiable.description.presence || 'Token'} was disabled"

"Token #{notifiable.description.presence || ''} was disabled".strip

Copilot · 2025-12-10T05:08:10Z

src/api/app/models/notification_token.rb

+class NotificationToken < Notification
+  def description
+    "Token #{notifiable.description.presence || 'Token'} was disabled"
+  end
+
+  def excerpt
+    event_payload['summary'] || 'Token was disabled due to authorization failure'
+  end
+
+  def avatar_objects
+    [notifiable&.executor].compact
+  end
+
+  def link_text
+    'Token'
+  end
+
+  def link_path
+    return if notifiable.blank?
+
+    Rails.application.routes.url_helpers.token_path(notifiable)
+  end
+end


The NotificationToken class lacks test coverage. Other similar notification classes in the codebase have comprehensive test coverage (see notification_spec.rb), but there are no tests for NotificationToken methods like description, excerpt, avatar_objects, link_text, and link_path.

Consider adding a test file at src/api/spec/models/notification_token_spec.rb to ensure these methods behave correctly, especially edge cases like when the notifiable is nil or when the token has no description.

Copilot · 2025-12-10T05:08:10Z

src/api/app/models/event/token_disabled.rb

+    #   GitHub workflow token disabled
+    def subject
+      vendor_map = { 'github' => 'GitHub', 'gitlab' => 'GitLab', 'gitea' => 'Gitea' }
+      vendor = vendor_map[payload['scm_vendor']] || payload['scm_vendor']&.capitalize


When payload['scm_vendor'] is nil or not in the vendor_map, the subject will have an awkward format. If the vendor is nil, the subject becomes " workflow token disabled" with a leading space.

Consider handling the nil case explicitly:

def subject vendor_map = { 'github' => 'GitHub', 'gitlab' => 'GitLab', 'gitea' => 'Gitea' } vendor = vendor_map[payload['scm_vendor']] || payload['scm_vendor']&.capitalize || 'SCM' "#{vendor} workflow token disabled" end

Suggested change

vendor = vendor_map[payload['scm_vendor']] || payload['scm_vendor']&.capitalize

vendor = vendor_map[payload['scm_vendor']] || payload['scm_vendor']&.capitalize || 'SCM'

Copilot · 2025-12-10T05:08:10Z

src/api/spec/models/event/token_disabled_spec.rb

+  describe '#subject' do
+    subject { event.subject }
+
+    let(:token) { create(:workflow_token) }
+
+    context 'with GitHub vendor' do
+      let(:event) do
+        Event::TokenDisabled.create(
+          token_id: token.id,
+          scm_vendor: 'github',
+          summary: 'Failed to report back to GitHub: Unauthorized request.',
+          token_description: 'My workflow token'
+        )
+      end
+
+      it { expect(subject).to eq('GitHub workflow token disabled') }
+    end
+
+    context 'with GitLab vendor' do
+      let(:event) do
+        Event::TokenDisabled.create(
+          token_id: token.id,
+          scm_vendor: 'gitlab',
+          summary: 'Failed to report back to GitLab: Request forbidden.',
+          token_description: 'My workflow token'
+        )
+      end
+
+      it { expect(subject).to eq('GitLab workflow token disabled') }
+    end
+  end


Test coverage is missing for edge cases in the #subject method:

When scm_vendor is nil

When scm_vendor is an unknown vendor (not in vendor_map)

When scm_vendor is 'gitea' (which is supported but not tested)

Consider adding test cases for these scenarios to ensure the subject method handles all possible inputs correctly.

Copilot · 2025-12-10T05:08:11Z

src/api/spec/models/event/token_disabled_spec.rb

+RSpec.describe Event::TokenDisabled do
+  describe '#token_executors' do
+    subject { event.token_executors }
+
+    let(:token) { create(:workflow_token) }
+    let(:event) do
+      Event::TokenDisabled.create(
+        token_id: token.id,
+        scm_vendor: 'github',
+        summary: 'Failed to report back to GitHub: Unauthorized request.',
+        token_description: 'My workflow token'
+      )
+    end
+
+    it { expect(subject).to contain_exactly(token.executor) }
+
+    context 'when the token does not exist' do
+      before do
+        event
+        token.destroy
+      end
+
+      it { expect(subject).to be_empty }
+    end
+  end


Test coverage is incomplete for Event::TokenDisabled. Missing tests for:

parameters_for_notification method (line 23-25)

event_object method (line 27-29)

token_members receiver role functionality (line 7)

Consider adding tests to verify these methods work correctly, especially the parameters_for_notification method which sets up the notification creation with the correct type and notifiable.

When a Token::Workflow fails to deliver to the SCM with authorization errors ('Unauthorized request' or 'Request forbidden'), the token gets disabled. This change adds: - Event::TokenDisabled: New event triggered when token is disabled - NotificationToken: Model for rendering token-related notifications - Notification system: Sends notifications to token executor and members - Default subscriptions: Enabled by default for instant_email and web channels - Comprehensive tests: Full coverage for all event and notification methods - Proper brand names: GitHub, GitLab, Gitea displayed correctly - Safe token lookup: Uses find_by instead of find to prevent exceptions - Nil-safe vendor handling: Falls back to 'SCM' when vendor is nil The notification helps users immediately understand why their token was disabled and take corrective action. Fixes openSUSE#18886

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-10T07:48:06Z

src/api/app/models/event/token_disabled.rb

+    end
+
+    def event_object
+      Token.find_by(id: payload['token_id'])


The event_object method searches for any Token type, but this event is specific to workflow tokens. This is inconsistent with the token private method (line 33-34) which correctly searches for Token::Workflow. This could return the wrong token type if a non-workflow token has the same ID.

Consider changing this to:

def event_object Token.find_by(id: payload['token_id'], type: 'Token::Workflow') end

Or reuse the existing private method:

def event_object token end

Suggested change

Token.find_by(id: payload['token_id'])

token

Copilot AI review requested due to automatic review settings December 7, 2025 09:56

github-actions bot added the Frontend Things related to the OBS RoR app label Dec 7, 2025

Copilot started reviewing on behalf of geetxnshgoyal December 7, 2025 09:57 View session

Copilot AI reviewed Dec 7, 2025

View reviewed changes

geetxnshgoyal force-pushed the fix-token-circuit-breaker-18886 branch from 4c5b07b to ba752c8 Compare December 7, 2025 10:18

krauselukas requested changes Dec 8, 2025

View reviewed changes

geetxnshgoyal force-pushed the fix-token-circuit-breaker-18886 branch from ba752c8 to b549f85 Compare December 10, 2025 04:23

geetxnshgoyal force-pushed the fix-token-circuit-breaker-18886 branch from b549f85 to 7f0e2ac Compare December 10, 2025 04:39

geetxnshgoyal requested review from Copilot and krauselukas December 10, 2025 04:58

Copilot started reviewing on behalf of geetxnshgoyal December 10, 2025 05:04 View session

Copilot AI reviewed Dec 10, 2025

View reviewed changes

geetxnshgoyal force-pushed the fix-token-circuit-breaker-18886 branch 2 times, most recently from c70562a to 69709ce Compare December 10, 2025 06:18

geetxnshgoyal force-pushed the fix-token-circuit-breaker-18886 branch from 69709ce to 6e79dc3 Compare December 10, 2025 07:24

geetxnshgoyal requested a review from Copilot December 10, 2025 07:43

Copilot started reviewing on behalf of geetxnshgoyal December 10, 2025 07:44 View session

Copilot AI reviewed Dec 10, 2025

View reviewed changes

	Token.find(payload['token_id'])
	Token.find_by(id: payload['token_id'])

-  def up; end
+  def up
+    # Find the event type for TokenDisabled
+    event_type = EventType.find_by(name: 'TokenDisabled')
+    return unless event_type
+    # For each user, create default subscriptions for instant_email and web channels if not already present
+    User.find_each do |user|
+      %w[instant_email web].each do |channel|
+        EventSubscription.find_or_create_by!(
+          user_id: user.id,
+          event_type_id: event_type.id,
+          channel: channel
+        )
+      end
+    end
+  end

	"Token #{notifiable.description.presence \|\| 'Token'} was disabled"
	"Token #{notifiable.description.presence \|\| ''} was disabled".strip

	vendor = vendor_map[payload['scm_vendor']] \|\| payload['scm_vendor']&.capitalize
	vendor = vendor_map[payload['scm_vendor']] \|\| payload['scm_vendor']&.capitalize \|\| 'SCM'

Add token circuit breaker notification (issue #18886) #18954

Are you sure you want to change the base?

Add token circuit breaker notification (issue #18886) #18954

Conversation

geetxnshgoyal commented Dec 7, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krauselukas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geetxnshgoyal commented Dec 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

krauselukas left a comment •

edited

Loading