Fix database connection leaks in DEI routes and user CLI commands
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()anddei_report()to use context managers for automatic session cleanup - Added try-finally blocks to
add_user()andreset_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 explicitsession.commit()to persist BadgingDEI records
Notes for Reviewers
dei_report()scoping is critical: session must stay open during template rendering (lazy-loadsproject.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 addandaugur user password_resetcommands - 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.
- 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.
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 are there tests to confirm that the core of this change does in fact leak fewer connections?
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?