Check and drop if stale connection when saving task result in db#498
Check and drop if stale connection when saving task result in db#498lode-braced wants to merge 5 commits intocelery:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds protection against stale database connections when storing Celery task results. It calls Django's close_if_unusable_or_obsolete() method before saving results to ensure the connection is still valid, particularly for long-running tasks that may exceed the configured connection max age.
Key Changes:
- Added connection validation logic in
_store_result()to close stale connections before saving - Added test coverage to verify stale connections are properly detected and replaced
- Configured
CONN_MAX_AGE: Nonefor test databases to enable persistent connection testing
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
django_celery_results/backends/database.py |
Added close_if_unusable_or_obsolete() call and imported connections from django.db |
t/unit/backends/test_database.py |
Added test to validate stale connection handling, imported time and connections |
t/proj/settings.py |
Configured CONN_MAX_AGE: None for all test database connections |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@auvipy the copilot description beat me to the punch: I noticed some of our very long running celery tasks getting this exception - the work they do is not on the db connection of the task thread, and so that connection remains idle until the end of the task, at which point the server already kicked it off for being idle for too long. Main consequence is that I had to modify the tests to run outside of the usual pytest-django transaction, since the connection dropping does not play nice with being inside a transaction. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| db_conn_wrapper = connections[router.db_for_write(self.b.TaskModel)] | ||
| db_conn_wrapper.close_at = time.monotonic() | ||
| current_db_connection = db_conn_wrapper.connection | ||
| self.b.mark_as_done(tid, None, request=request) | ||
| # Validate the connection was replaced in the process | ||
| # of saving the task | ||
| assert current_db_connection is not db_conn_wrapper.connection |
There was a problem hiding this comment.
This test captures current_db_connection = db_conn_wrapper.connection before ensuring a DB connection is actually established. If connection is None at that point (lazy connection creation), the assertion will pass even if close_if_unusable_or_obsolete() never replaced an existing connection. Force the connection to be opened before recording it (e.g., ensure/connect/cursor) so the test genuinely validates replacement of a stale, existing connection.
|
|
||
|
|
||
| @pytest.mark.django_db() | ||
| @pytest.mark.django_db(transaction=True) |
There was a problem hiding this comment.
Changing the class-level marker to @pytest.mark.django_db(transaction=True) makes every test in this (large) suite run with transactional DB behavior (TransactionTestCase-style), which is significantly slower and broader than necessary. Consider limiting transaction=True to only the new stale-connection test (or any tests that truly require it) to avoid slowing down unrelated tests.
| @pytest.mark.django_db(transaction=True) | |
| @pytest.mark.django_db |
No description provided.