augur icon indicating copy to clipboard operation
augur copied to clipboard

Fix database connection leaks in DEI routes and user CLI commands

Open shlokgilda opened this issue 1 month ago • 4 comments

Description

  • Fixed database connection leaks in DEI API endpoints and user CLI commands. Fixes #3401.
  • Three functions were creating database sessions without properly closing them in all code paths (early returns, exceptions)
  • Converted dei_track_repo() and dei_report() to use context managers for automatic session cleanup
  • Added try-finally blocks to add_user() and reset_password() CLI commands to guarantee session cleanup
  • Fixed missing import in dei_track_repo()
  • Removed non-functional session.autocommit = True (has no effect in SQLAlchemy 2.x) and added explicit session.commit() to persist BadgingDEI records

Notes for Reviewers

  • dei_report() scoping is critical: session must stay open during template rendering (lazy-loads project.repo), then closes before file operations
  • All changes follow existing context manager patterns used elsewhere in the codebase (e.g., augur/application/cli/db.py)
  • Manually tested augur user add and augur user password_reset commands - sessions properly close in all paths
  • DEI endpoints (dei_track_repo, dei_report) should be manually tested (although I don't think these are even used?)

Signed commits

  • [x] Yes, I signed my commits.

shlokgilda avatar Nov 17 '25 03:11 shlokgilda

  • to persist BadgingDEI records DEI endpoints (dei_track_repo, dei_report) should be manually tested (although I don't think these are even used?)

i think that table is unused and i plan to drop it soon lol (just filed https://github.com/chaoss/augur/issues/3423) I suspect you are right about the routes too.

Fixed database connection leaks in DEI API endpoints and user CLI commands

funny you say that. I'm fairly sure that the DatabaseSession object itself is causing leaks, even when used with with based on chatting with other maintainers. Gen AI suggests that this is because the methods often returns raw Result objects due to db calls not using functions like .one() that specify what kind of results they want, meaning connections are held onto and brought outside the with scope.

Overall it seems like you are trying to do several things in this PR that may be better to split into separate, more focused PRs.

MoralCode avatar Nov 19 '25 22:11 MoralCode

Okay, yeah! The "too many things in one PR" sounds like me.

I'll try to be more vigilant of this.

Re DatabaseSession: I didn't check that. If that's the case, that should fix a whole bunch of issues.

shlokgilda avatar Nov 19 '25 23:11 shlokgilda

@shlokgilda are there tests to confirm that the core of this change does in fact leak fewer connections?

MoralCode avatar Dec 11 '25 14:12 MoralCode

Hmm, good question! I don't think so. Might have to figure out a way to calculate the number of connections before/after the fix.

Any suggestions?

shlokgilda avatar Dec 11 '25 15:12 shlokgilda