Skip to content

Fix/FabAuthManager race condition on startup with multiple workers#62737

Merged
potiuk merged 7 commits intoapache:mainfrom
Pyasma:pyasma/FabAuthManager
Mar 5, 2026
Merged

Fix/FabAuthManager race condition on startup with multiple workers#62737
potiuk merged 7 commits intoapache:mainfrom
Pyasma:pyasma/FabAuthManager

Conversation

@Pyasma
Copy link
Contributor

@Pyasma Pyasma commented Mar 2, 2026

issue : #62563
When running the API server with multiple workers, they all try to initialize FAB permissions at exactly the same time, which causes a database race condition.

This PR simply catches that IntegrityError to safely ignore it, since having multiple workers trying to add the exact same permission is actually fine. Also added 2 tests to verify this logic works.

prek checks passed


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

…o prevent erroneous logging for concurrent duplicate permission assignments and add a helper method to check for existing permissions
@Pyasma Pyasma requested a review from vincbeck as a code owner March 2, 2026 20:53
@Pyasma Pyasma marked this pull request as draft March 2, 2026 20:55
@Pyasma Pyasma marked this pull request as ready for review March 2, 2026 20:56
@Pyasma
Copy link
Contributor Author

Pyasma commented Mar 2, 2026

@jason810496 @Lee-W can you please review this

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Addresses a startup race condition when running the API server with multiple workers using FabAuthManager, where concurrent permission-role initialization can trigger DB IntegrityErrors.

Changes:

  • Catch sqlalchemy.exc.IntegrityError in add_permission_to_role and suppress logging when the permission-role association already exists after rollback.
  • Add _is_permission_assigned_to_role helper to verify whether the association exists in the DB.
  • Add unit tests covering both the “ignore duplicate” and “log error when not persisted” cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py Adds IntegrityError handling and DB verification to avoid noisy logs during concurrent FAB permission initialization.
providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py Adds unit tests validating the new duplicate-handling behavior and the fallback error logging behavior.

@Pyasma Pyasma requested a review from jedcunningham March 3, 2026 12:12
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

nit but overall looks good

@Pyasma Pyasma requested a review from Lee-W March 3, 2026 14:35
Copy link
Member

@jason810496 jason810496 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. LGTM overall.

@potiuk potiuk added this to the Airflow 3.1.8 milestone Mar 5, 2026
@potiuk potiuk merged commit 5c951d9 into apache:main Mar 5, 2026
235 of 238 checks passed
@eladkal eladkal removed this from the Airflow 3.1.9 milestone Mar 5, 2026
1Ninad pushed a commit to 1Ninad/airflow that referenced this pull request Mar 6, 2026
…pache#62737)

* fix: Gracefully handle `IntegrityError` in `add_permission_to_role` to prevent erroneous logging for concurrent duplicate permission assignments and add a helper method to check for existing permissions

* fix: Correctly handle IntegrityError on session commit when adding permissions to roles and improve permission check query.

* refactor: optimize permission role existence check query using `sqlalchemy.exists()`

* use  for assertions

* changing error log message

* change log from debug to info
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants