Close database connections on every entrypoint - #879
Conversation
Celery workers opened a database per task and never closed it, so a long-running worker churned a connection for every result it handled. Workers now open one database per process and reuse it, keyed on the pid so a forked child never uses connections it inherited. Also closes the databases the executors open for themselves, and the one the standalone alembic environment opens. An execution still in flight when `LocalExecutor.join` raises is now marked failed-retryable, which is what the docstring already promised. Only the timeout path did this before, so an ingestion failure left the remaining rows stuck with `successful=None` until the stale-execution reaper caught them hours later.
|
Warning Review limit reached
Next review available in: 18 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe changes add process-aware database reuse for Celery workers, finalise internally created databases, and improve local executor cleanup. Local executor failures now mark outstanding executions as failed retryable before re-raising the error. ChangesResource and retry lifecycle
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/climate-ref-celery/tests/unit/test_worker_tasks.py (1)
11-16: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winProve both database close paths in the tests.
The fixture calls
_worker_database.close()directly, so it does not verify theworker_process_shutdowncallback. The URL-change test verifies only replacement and the URL; it does not verify that the replacedDatabasewas closed. Add a spy or mock forDatabase.close, then exercise URL replacement and_close_worker_database.Also applies to: 25-32
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 072a6641-3d94-4589-921a-f91675087f2f
📒 Files selected for processing (8)
changelog/879.fix.mdpackages/climate-ref-celery/src/climate_ref_celery/worker_tasks.pypackages/climate-ref-celery/tests/unit/test_worker_tasks.pypackages/climate-ref/src/climate_ref/executor/hpc.pypackages/climate-ref/src/climate_ref/executor/local.pypackages/climate-ref/src/climate_ref/executor/synchronous.pypackages/climate-ref/src/climate_ref/migrations/env.pypackages/climate-ref/tests/unit/executor/test_local_executor.py
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
An execution abandoned before a worker picked it up has no output directory, so the missing log file is expected rather than a system error worth shouting about. Reports it at debug in that case, and keeps the error for an execution that did start. Also adds the missing space between the two halves of the uncollected-execution warning.
| time.sleep(refresh_time) | ||
| except BaseException: | ||
| # Handle Ctrl-C and SystemExit exceptions to mark in-flight executions as retryable | ||
| self._fail_outstanding(results, t) |
There was a problem hiding this comment.
@fuchsi-huber This should resolve the Ctrl-C interrupt leaving the in flight diagnostics requirinng manual intervention
There was a problem hiding this comment.
Oh, nice! That is definitely a QoL improvement :)
2 participants
Audits how every entrypoint manages its database session and connection, then fixes the four problems that turned up. The CLI was already correct, and
Database.session_scopeholds up under concurrent threads, so neither changes here.Celery workers opened a
Databaseper task and never closed it. On SQLite the garbage collector eventually mopped up, so open files plateaued around ten. On Postgres it means a fresh connection for every result a worker handles. Workers now open one database per process and reuse it across tasks, keyed on the pid so a forked child opens its own rather than using connections it inherited. Aworker_process_shutdownhook closes it. Measured over 100 tasks, open files drop from 10 to 1.The three executors open a database of their own when the caller does not supply one, and never closed it. That path is not used in production, where the CLI always passes its database in, but it leaks for anyone constructing an executor by hand. Each now closes what it opened. The standalone alembic environment gets the same treatment.
LocalExecutor.joinpromised that outstanding executions are always marked failed-retryable before it returns or raises. Only the timeout path did this, so an ingestion failure mid-loop left the remaining rows withsuccessful=Noneuntil the stale-execution reaper caught them six hours later. The promise now holds for every failure, and the pool is shut down on the way out.Verified with three live experiments rather than by reading alone: file-descriptor counts across 100 simulated tasks, eight threads hammering
session_scopeon SQLite (0 errors), and a spy onDatabase.closeconfirming the CLI closes on success, ontyper.Exit, and on an unexpected exception.Adds three tests.
packages/climate-refandpackages/climate-ref-celerypass in full at 1735 tests.Summary by CodeRabbit
Bug Fixes
Performance
Documentation