fix(db): rollback on SessionCTX exception + pool_pre_ping#20
Merged
Conversation
SessionCTX.__exit__ now rolls back the session on exception before closing, so dirty connections never return to the pool. The close path was reworked under try/finally so a failure during flush/close cannot leave _session_cnt stuck above zero (previously leaked the session forever). close_session() gained a skip_flush flag used on the exception path to avoid flush re-raising and masking the original error. pool_pre_ping is now enabled on both the standard MySQL/Postgres engine and the Cloud SQL IAM engine (skipped for SQLite). This catches Cloud SQL's silent idle-disconnects before the next query instead of surfacing "server has gone away" to the user. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eError reattach() in SampleBrowserModel previously caught DetachedInstanceError and killed the app. Root cause: get_analyses_uuid returned ORM rows with default lazy='select' chains (irradiation_position.level/sample.project) that detached when session_ctx closed. Downstream callers (progress_bind_records -> AnalysisTbl.bind()) then walked those chains and raised. Add joinedload/selectinload options matching get_labnumber_analyses, extracted as _analysis_eager_options() helper, and apply to get_analysis, get_analysis_uuid, get_analyses_uuid, get_analysis_runid, and get_analysis_by_attr. Drop the try/except in reattach() so any remaining bug surfaces instead of a "restart pychron" dialog. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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
__exit__now rolls back the session before close when an exception propagates through the context, so dirty connections never return to the pool with a half-committed transaction. Previously this could surface as a phantom failure several call sites away from the original error.close_sessiondecrement moved into afinallyblock. An exception during flush/close no longer leaves_session_cntstuck above zero (which would leak the session forever and causeDetachedInstanceError-adjacent symptoms downstream).pool_pre_ping=Trueenabled on both the standard MySQL/Postgres engine and the Cloud SQL IAM engine (skipped for SQLite). Catches Cloud SQL idle-disconnects before the next query instead of surfacing "server has gone away" to the user.Why
The user reported intermittent
DetachedInstanceErrors and addedsession_ctxas a workaround. Audit of the context manager surfaced three robustness gaps that this PR addresses:__exit__closed without rollback on exception — dirty connection returned to pool.close_sessiondecremented the refcount afterflush(), so a flush exception left the counter stuck and the session permanently held.pool_pre_ping=False, so idle-drops surfaced as errors instead of being silently recovered.The
expire_on_commit=Falsesetting and the application-level catch insample_browser_model.pymask symptoms; this PR fixes upstream causes.What this does NOT do
Deferred to follow-up tasks (already spawned):
DetachedInstanceErrorcatch insample_browser_model.py(reattach()kills the app) — root cause isget_analyses_uuidreturning ORM rows with lazy relationships. Needs eager-load or DTO conversion.make_analyses,repository_transfer, sample browser load). Needs profiling first.scoped_sessionmigration — no background DB threads found in the codebase, so the current refcount + lock model is adequate. Revisit if threading needs arise.Test plan
session_ctx— counter increments/decrements correctly, session closes on outermost exitsession_ctx— session rolls back, counter resets to 0, session attribute clearedsession_ctx— outer survives with counter back to 1, closes cleanly on outer exituse_parent_session=False— separate session created, parent session restored on exit🤖 Generated with Claude Code