Skip to content

OSW-2388: Fix database connection pool exhaustion in ConsDB pqserver insert path.#120

Merged
bbrondel merged 1 commit into
mainfrom
tickets/OSW-2388
Jun 3, 2026
Merged

OSW-2388: Fix database connection pool exhaustion in ConsDB pqserver insert path.#120
bbrondel merged 1 commit into
mainfrom
tickets/OSW-2388

Conversation

@bbrondel
Copy link
Copy Markdown
Collaborator

@bbrondel bbrondel commented Jun 3, 2026

This change fixes connection cleanup in the helper functions that cross reference exposure ID to day_obs/seq_num/[detector].

Two other bugs are also addressed, for a cached lookup and for a formatted print statement.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses connection pool exhaustion in the pqserver insert path by ensuring DB sessions created via the get_db() generator are deterministically closed when used outside FastAPI’s dependency lifecycle. It also fixes a cached instrument schema lookup bug and corrects a logging statement to actually log the incoming SQL query.

Changes:

  • Ensure InstrumentTable helper methods explicitly close the get_db() generator so sessions/connections are returned to the pool immediately.
  • Fix get_instrument_list() to correctly populate and return the module-level cached _instrument_list.
  • Fix pqserver query endpoint logging to log data.query using parameterized logging.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
python/lsst/consdb/handlers/external.py Fixes query endpoint logging to correctly log the provided SQL query.
python/lsst/consdb/dependencies.py Fixes instrument list caching by using the module-level _instrument_list.
python/lsst/consdb/cdb_schema.py Ensures DB sessions from get_db() are properly closed in helper lookup paths to prevent connection leaks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Collaborator

@Vebop Vebop left a comment

Choose a reason for hiding this comment

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

Definitely an improvement, please consider adding the context manager

Comment thread python/lsst/consdb/cdb_schema.py Outdated
finally:
# Run the generator's finally (session.close()) so the pooled
# connection is returned rather than leaked until GC.
db_gen.close()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why not a context manager here? That way the db connection always calls db_gen.close if there's an error on 189

from contextlib import contextmanager

@contextmanager
def _session(self):
    try:
        yield next(self.get_db)
    finally:
        self.get_db.close()

# Usage anywhere in the class:
with self._session() as db:
    query_result = db.execute(query).first()

(Apply this suggestion to the following two edits in this file too.)

Comment thread python/lsst/consdb/dependencies.py Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change

@bbrondel bbrondel force-pushed the tickets/OSW-2388 branch from 3638f3f to f536669 Compare June 3, 2026 17:48
@bbrondel bbrondel merged commit cf81a08 into main Jun 3, 2026
12 checks passed
@bbrondel bbrondel deleted the tickets/OSW-2388 branch June 3, 2026 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants