CCMRG-1943: Move database connections from worker level to task level - #618
Closed
drazisil-codecov wants to merge 13 commits into
Closed
CCMRG-1943: Move database connections from worker level to task level#618drazisil-codecov wants to merge 13 commits into
drazisil-codecov wants to merge 13 commits into
Conversation
- Refactor database session management to use per-task sessions instead of shared worker-level sessions - Create create_task_session() function for isolated task sessions - Update BaseCodecovTask.run() to create new session per task - Replace wrap_up_dbsession() with wrap_up_task_session() for per-task cleanup - Update celery_task_router to use temporary sessions for routing - Fix test mocks to work with new per-task session architecture - Add debug logging to diagnose session binding issues in tests
- Introduced a test session factory to allow shared test sessions in unit tests. - Updated `save_commit_measurements` and related functions to accept an optional `db_session` parameter for better session management. - Refactored `upsert_components_measurements` and `repository_flag_ids` to utilize the provided `db_session`. - Improved error handling in `upsert_component` to raise informative exceptions for missing commits. - Enhanced logging and session binding checks in various tasks to ensure proper session visibility during tests.
Contributor
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #618 +/- ##
==========================================
+ Coverage 93.81% 93.88% +0.07%
==========================================
Files 1286 1285 -1
Lines 46788 46891 +103
Branches 1522 1522
==========================================
+ Hits 43892 44025 +133
+ Misses 2586 2556 -30
Partials 310 310
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
added 6 commits
December 18, 2025 09:45
…er-task session implementation
… clarity and consistency. Removed redundant assertions and streamlined the verification of call arguments in mocked super method calls.
drazisil-codecov
marked this pull request as ready for review
December 18, 2025 17:52
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…ion logic. Removed unnecessary conditional check for original_commit, ensuring dbsession.commit is always reset correctly.
…ling. Introduced try-finally structure for session cleanup, improved transaction validation, and streamlined retry logic to ensure robust handling of exceptions during task execution.
| _ = session_factory.create_session() | ||
|
|
||
| main_engine = session_factory.main_engine | ||
| timeseries_engine = session_factory.timeseries_engine |
Contributor
There was a problem hiding this comment.
Is this also for TA timeseries? We have 3 engines don't we?
Contributor
Author
There was a problem hiding this comment.
Per line 80 we only have two. 🤔
|
|
||
| def update_upload(self, carriedforward: bool | None = False) -> None: | ||
| def update_upload( | ||
| self, carriedforward: bool | None = False, db_session: DbSession | None = None |
Contributor
There was a problem hiding this comment.
Might have missed it or might see it later in the review, but why do we need to pass the session as a param now?
Contributor
Author
There was a problem hiding this comment.
Because we aren't using global Celery session, and also injectable for testing.
added 2 commits
December 19, 2025 13:47
…up and enhance error handling. Updated try-finally structure to account for session creation failures, ensuring that the task session is always cleaned up if created.
- Add debug logging for session creation/cleanup in apply_async and run - Log session_id, task name, and whether transaction was open - Preserve attempts header from PR #633 to fix visibility timeout tracking - Logging helps verify session cleanup is working correctly in production
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
drazisil-codecov
pushed a commit
that referenced
this pull request
Jan 7, 2026
- Update test_commit_measurement_update_component_parallel to use hook_session - Update test_delete_repository_data_measurements_only to use hook_session - Update test_compute_component_comparisons_parallel to use hook_session - Pass db_session parameter to save_commit_measurements calls
drazisil-codecov
pushed a commit
that referenced
this pull request
Jan 7, 2026
…components_measurements These functions need the db_session parameter to work with per-task sessions, matching the pattern used in PR #618.
Contributor
Author
|
Closing as won't do - the scope of this change (per-task database sessions) is significant and the user impact from the related errors doesn't justify the risk/effort at this time. The linked issue CCMRG-1943 has also been closed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors database session management to use per-task sessions instead of shared worker-level sessions. This prevents transaction contamination between tasks and eliminates the risk of hitting
idle_in_transaction_session_timeoutwhen tasks fail or timeout.Changes
create_task_session()function for isolated task sessionsBaseCodecovTask.run()to create new session per taskwrap_up_dbsession()withwrap_up_task_session()for per-task cleanupcelery_task_routerto use temporary sessions for routingBenefits
idle_in_transaction_session_timeoutissuesTesting
create_task_sessioninstead ofget_db_sessioncommit.get_db_session()usingtypes.MethodTypeNote
Refactors worker DB access to use isolated per-task SQLAlchemy sessions and makes routing lookups use short‑lived sessions to avoid transaction bleed.
TaskSessionManagerwithcreate_task_session()and test override hook; adds unit tests for engines/sessionsBaseCodecovTask.apply_async/runto open/close a per-task session, commit on success, rollback on errors, and replacewrap_up_dbsessionwithwrap_up_task_session; preserves/mergesattemptsheaderdb_sessionthrough report/test-results/timeseries/upload code paths (e.g.,initialize_and_save_report,create_report_upload,save_report, component upsert) and uses it where availablerepository_flag_ids,upsert_components_measurements) to accept/forward sessionsLogContext.populate_from_sqlalchemyresilient to missing rowsWritten by Cursor Bugbot for commit 1bdb8ef. This will update automatically on new commits. Configure here.