Skip to content

Add pool_pre_ping option to SQLAlchemy engine configuration - #8549

Open
MichalTorma wants to merge 1 commit into
specify:mainfrom
MichalTorma:fix-stale-sqlalchemy-pool-after-idle
Open

MichalTorma wants to merge 1 commit into
specify:mainfrom
MichalTorma:fix-stale-sqlalchemy-pool-after-idle

Conversation

@MichalTorma

@MichalTorma MichalTorma commented Sep 17, 2026

Copy link
Copy Markdown

Updated SQLAlchemy engine initialization across multiple files to include the pool_pre_ping option, enhancing connection reliability by preemptively checking for closed connections. This change was made in models.py, sp7_build_models.py, and tests.py, and the SA_POOL_PRE_PING setting was added to the configuration in init.py.

Fixes #

Checklist

  • Self-review the PR after opening it to make sure the changes look good and
    self-explanatory (or properly documented)
  • Add relevant issue to release milestone
  • Add pr to documentation list
  • Add automated tests
  • Add a reverse migration if a migration is present in the PR
  • Add migration function to
    def fix_schema_config(stdout: WriteToStdOut | None = None):

Testing instructions

Summary by CodeRabbit

  • Bug Fixes
    • Improved database connection reliability by checking connections before use and replacing stale connections automatically.
    • Reduced failures caused by connections closed after extended inactivity.

Updated SQLAlchemy engine initialization across multiple files to include the pool_pre_ping option, enhancing connection reliability by preemptively checking for closed connections. This change was made in models.py, sp7_build_models.py, and tests.py, and the SA_POOL_PRE_PING setting was added to the configuration in __init__.py.
@coderabbitai

coderabbitai Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: abeeb756-5ab8-443b-bf52-7497f978cf21

📥 Commits

Reviewing files that changed from the base of the PR and between 2c3012d and e48c956.

📒 Files selected for processing (4)
  • specifyweb/backend/stored_queries/models.py
  • specifyweb/backend/stored_queries/sp7_build_models.py
  • specifyweb/backend/stored_queries/tests/tests.py
  • specifyweb/settings/__init__.py

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

The change adds SA_POOL_PRE_PING = True and passes it to runtime, generated, and test SQLAlchemy engine constructors.

Changes

SQLAlchemy pool pre-ping

Layer / File(s) Summary
Pool pre-ping setting
specifyweb/settings/__init__.py
Adds SA_POOL_PRE_PING = True and updates the pool configuration comment.
Engine constructor integration
specifyweb/backend/stored_queries/models.py, specifyweb/backend/stored_queries/sp7_build_models.py, specifyweb/backend/stored_queries/tests/tests.py
Passes settings.SA_POOL_PRE_PING to runtime, generated, and test SQLAlchemy create_engine calls.

Priority: ⬇️ Low

Change: Feature

Merge Risk: ⚪ Minimal · up to e48c9

This change enables stale-connection checks across runtime, generated, and test engines without an identified merge-blocking risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Automatic Tests ⚠️ Warning The PR adds a new connection-pool behavior, but it adds no automatic test that verifies the behavior. The only test-file change updates the shared setup_sqlalchemy helper to pass pool_pre_ping; it… Add automatic tests for the new configuration. Assert that the runtime SQLAlchemy engines use settings.SA_POOL_PRE_PING, and add generator coverage that the code produced by gen_sqlalchemy_table_classes_code includes the pool_pre_ping
Testing Instructions ⚠️ Warning The PR has no testing instructions. Its ### Testing instructions section contains only the template comments. The diff changes SQLAlchemy engine creation in models.py, generated engine code in `sp… Add clear testing instructions to the PR description. State the backend test command, including the stored-query tests that exercise setup_sqlalchemy, and state the expected result. Add a check that SA_POOL_PRE_PING is enabled and that …
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding the pool_pre_ping option to SQLAlchemy engine configuration.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Automatic Tests

Explanation

The PR adds a new connection-pool behavior, but it adds no automatic test that verifies the behavior. The only test-file change updates the shared setup_sqlalchemy helper to pass pool_pre_ping; it does not assert the engine configuration. Existing SQLAlchemySetup tests exercise queries, but they do not verify pre-ping, and no test covers the generated engine code in sp7_build_models.py. The contributor checklist also leaves “Add automated tests” unchecked.

Resolution

Add automatic tests for the new configuration. Assert that the runtime SQLAlchemy engines use settings.SA_POOL_PRE_PING, and add generator coverage that the code produced by gen_sqlalchemy_table_classes_code includes the pool_pre_ping setting. Keep the existing query integration tests unchanged as broader regression coverage.

Full details: Testing Instructions

Explanation

The PR has no testing instructions. Its ### Testing instructions section contains only the template comments. The diff changes SQLAlchemy engine creation in models.py, generated engine code in sp7_build_models.py, test-engine setup in tests.py, and adds the SA_POOL_PRE_PING setting. The repository workflow shows a backend test command, but the PR does not state any command, expected result, stale-connection/pre-ping check, or verification for the generated engine code and setting.

Resolution

Add clear testing instructions to the PR description. State the backend test command, including the stored-query tests that exercise setup_sqlalchemy, and state the expected result. Add a check that SA_POOL_PRE_PING is enabled and that the runtime engine uses it. Regenerate or inspect the generated SQLAlchemy model code and confirm that it includes pool_pre_ping=settings.SA_POOL_PRE_PING. If possible, verify that a stale pooled MySQL connection is detected and replaced before a query executes.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 📋Back Log

Development

Successfully merging this pull request may close these issues.

1 participant