Skip to content

Make database heartbeat more robust #21734

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mvdbeek merged 2 commits into from
Feb 10, 2026
+7 −2
Merged

Make database heartbeat more robust #21734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f458da
Make database heartbeat more robust
mvdbeek Feb 3, 2026
e69ff8c
Fix connection leak in check_database_connection
mvdbeek Feb 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/galaxy/model/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def check_database_connection(session):
if isinstance(session, scoped_session):
session = session()
trans = session.get_transaction()
if (trans and not trans.is_active) or session.connection().invalidated:
if trans and (not trans.is_active or session.connection().invalidated):
Copy link
Copy Markdown
Member Author

@mvdbeek mvdbeek Feb 4, 2026

Choose a reason for hiding this comment

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

@jdavcs can you sanity check this ? the integration tests were causing postgres to run out of connections prior to this change, supposedly because session.connection() checking out a connection and then never close it, so now we only check if there's an existing transaction.

Copy link
Copy Markdown
Member

@jdavcs jdavcs Feb 9, 2026

Choose a reason for hiding this comment

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

I don't think this affects anything: if condition satisifes, we only do session.rollback(), and accoriding to the docs, if no transaction is in progress, this method is a pass-through.(ref + ref). So in the current version (before this change), it would either rollback an existing transaction or do nothing, after the change it will do exactly the same thing.

I didn't find any mention of a connection-related side effect in case of a noop rollback, but I didn't dig too deep. That said, the edit doesn't do any harm, so if you think it's worth a try, that's fine.

Copy link
Copy Markdown
Member Author

@mvdbeek mvdbeek Feb 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't this what happens:

if session.connection().invalidated and invalidated is False we don't enter the branch but we have created a new connection that never gets cleaned up ?

Why else would this fix the integration tests ?

Copy link
Copy Markdown
Member Author

@mvdbeek mvdbeek Feb 9, 2026

Choose a reason for hiding this comment

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

Also see the commit message for a different phrasing in case that's not clear ?

Copy link
Copy Markdown
Member

@jdavcs jdavcs Feb 10, 2026

Choose a reason for hiding this comment

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

Ooooh! You are right! We create a connection in the if condition test - in order to test whether it's invalidated! Because, as per docs, "Either the Connection corresponding to the current transaction is returned, or if no transaction is in progress, a new one is begun and the Connection returned". Yikes. This was subtle. And pretty terrible. Thanks!

session.rollback()
log.error("Database transaction rolled back due to inactive session transaction or invalid connection state.")

Expand Down
7 changes: 6 additions & 1 deletion lib/galaxy/model/database_heartbeat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
)

from galaxy.model import WorkerProcess
from galaxy.model.base import check_database_connection
from galaxy.model.orm.now import now

log = logging.getLogger(__name__)
Expand Down Expand Up @@ -106,7 +107,11 @@ def update_watcher_designation(self):
def send_database_heartbeat(self):
if self.active:
while not self.exit.is_set():
self.update_watcher_designation()
check_database_connection(self.sa_session)
try:
self.update_watcher_designation()
except Exception:
log.exception("Error sending database heartbeat for server '%s'", self.server_name)
self.exit.wait(self.heartbeat_interval)

def _delete_worker_process(self):
Expand Down
Loading