Skip to content

SEP-1843: The first request or Celery task after a database restart fails on a stale pooled connection - #1378

Open
peter-o-addo wants to merge 12 commits into
mainfrom
SEP-1843
Open

SEP-1843: The first request or Celery task after a database restart fails on a stale pooled connection#1378
peter-o-addo wants to merge 12 commits into
mainfrom
SEP-1843

Conversation

@peter-o-addo

@peter-o-addo peter-o-addo commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Enable pool pre-ping by default so stale connections after a database restart are replaced on checkout instead of failing the first request or Celery task.

  • app/core/db/config.py, app/core/celery/config.py: add POOL_PRE_PING / pool_pre_ping defaulting to True and always emit it into engine kwargs (API pools and Celery worker).
  • app/core/db/utils.py: update create_app_async_engine docstring to match the always-forwarded key.
  • changelog.d/SEP-1843.fixed.md, changelog.d/SEP-1843.config.md: record the fix and the new config keys.
  • tests/app/core/db/…, tests/app/core/celery/…: update exact-dict assertions and cover default-on, opt-out, and NullPool strip behaviour.

Tested

  • Confirm sep, inventory, tasks, and worker engines all report _pre_ping=True by default with sizing keys omitted when unset.
  • Confirm POOL_PRE_PING=False emits pool_pre_ping: False so an operator can restore the previous checkout behaviour.
  • Confirm that after a Postgres restart the first pooled checkout succeeds with pre-ping on, and fails then recovers on the second checkout with pre-ping off.

Checklist

  • New/modified functions have type hints and rST docstrings
  • New tests added for new features or bug fixes
  • All tests pass locally (make test)
  • Pre-commit hooks pass (make run-pre-commit)
  • Database migrations generated if models changed (make makemigrations)
  • User-facing changes documented (README, inline help, UI text)
  • Configuration changes documented with examples
  • Changelog fragment added under changelog.d/ if the change is user-facing (make changelog-add), or confirmed N/A (internal-only change, or a same-release-cycle fix for an unreleased sibling ticket)

@peter-o-addo peter-o-addo self-assigned this Aug 20, 2026
@peter-o-addo
peter-o-addo marked this pull request as ready for review August 20, 2026 07:31
Copilot AI balanced review requested due to automatic review settings August 20, 2026 07:31
@peter-o-addo
peter-o-addo requested review from a team and yyyyyyyan as code owners August 20, 2026 07:31
@peter-o-addo peter-o-addo added the qa in progress Someone is currently testing this PR - do not merge it label Aug 20, 2026
@peter-o-addo

Copy link
Copy Markdown
Contributor Author

1. Default pre-ping on all four engines

Inspected pool_engine_kwargs / PoolEngineOptions dumps and engine.pool._pre_ping on the live sep, inventory, tasks, and worker engines.

Result

{'pool_pre_ping': True}
{'pool_pre_ping': True}
sep True
inventory True
tasks True
worker True

Pass: all four engines report _pre_ping=True; sizing keys omitted when unset.


2. Opt-out

Built a DatabaseOptions with POOL_PRE_PING=False and printed pool_engine_kwargs.

Result

{'pool_pre_ping': False}

Pass: POOL_PRE_PING=false restores the previous checkout behaviour for that engine.


3. Database restart (core fix)

Against a Docker Postgres instance, warmed a pooled connection, restarted the database, then checked out twice with pre-ping on and once as a control with it off.

Result

=== WITH pre-ping (fix): pool_pre_ping=True ===
  warm: OK (connection now sitting in pool)
  restarting sep-1843-pg...
  pg ready again after 1 attempts
  post-restart checkout #1: OK
  post-restart checkout #2: OK

=== WITHOUT pre-ping (control): pool_pre_ping=False ===
  warm: OK (connection now sitting in pool)
  restarting sep-1843-pg...
  pg ready again after 1 attempts
  post-restart checkout #1: FAIL (DBAPIError: ... ConnectionDoesNotExistError:
    connection was closed in the middle of operation
    [SQL: SELECT 1])
  post-restart checkout #2: OK

=== SUMMARY ===
with pre_ping:    checkout1/checkout2 = OK/OK
without pre_ping: checkout1/checkout2 = FAIL:DBAPIError/OK
PASS

Pass: with pre-ping, the first checkout after restart succeeds. Without it, the first fails and the second recovers — the original bug.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Enables SQLAlchemy pool pre-ping by default for API and Celery database engines, replacing stale pooled connections after database restarts.

Changes:

  • Adds configurable pre-ping defaults and engine forwarding.
  • Adds unit coverage for defaults, opt-out, and NullPool behavior.
  • Documents the configuration and reliability change.

Reviewed changes

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

Show a summary per file
File Description
app/core/db/config.py Adds API database pre-ping configuration.
app/core/db/utils.py Updates engine-factory documentation.
app/core/celery/config.py Adds Celery engine pre-ping configuration.
tests/app/core/db/test_config.py Tests database option serialization.
tests/app/core/db/test_utils.py Tests async engine pre-ping wiring.
tests/app/core/celery/test_config.py Tests Celery configuration behavior.
tests/app/core/celery/test_db.py Tests worker engine forwarding.
tests/app/core/celery/test_beat_engine_pool.py Tests scheduler pool paths.
changelog.d/SEP-1843.fixed.md Records the stale-connection fix.
changelog.d/SEP-1843.config.md Documents the new configuration keys.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread app/core/db/config.py
Comment thread app/core/celery/config.py
Comment thread tests/app/core/db/test_config.py
Comment thread app/core/celery/config.py Outdated
Comment thread tests/app/core/celery/test_beat_engine_pool.py
Comment thread tests/app/core/celery/test_config.py
@peter-o-addo
peter-o-addo requested review from a team and maxbube as code owners August 20, 2026 08:45
@peter-o-addo peter-o-addo added qa passed Tests for this PR are completed and successful. and removed qa in progress Someone is currently testing this PR - do not merge it labels Aug 20, 2026
@github-actions

Copy link
Copy Markdown

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  app/core/celery
  config.py
  app/core/db
  config.py
  utils.py
  app/sep/sync/syncers
  pmm.py
Project Total  

This report was generated by python-coverage-comment-action

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

Labels

python qa passed Tests for this PR are completed and successful.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants