fix(py): scope server(data_source=) cleanup and table guards to live sessions - #308
Merged
Merged
Conversation
cpsievert
added a commit
that referenced
this pull request
Sep 12, 2026
…ation Replace the .server_initialized boolean with an .active_sessions counter (incremented on (), decremented via session()): - (data_source=) cleans up the source/executor it replaces whenever no session is live -- including a later session whose predecessor has fully ended, which the boolean treated as permanently uncleanable. - ()/()/() are blocked only while a session is actually active, rather than forever after the first () call. Fails safe: if onSessionEnded never fires, behavior degrades to the previous over-conservative semantics, never premature cleanup. Mirrors #308 (Python).
cpsievert
changed the title
…sessions Track live sessions on the instance (_active_sessions, incremented when a session's server starts, decremented via session.on_ended) and key shared-state decisions off it: - server(data_source=) cleans up the source/executor it replaces exactly when no session is live (cleanup_replaced = _active_sessions == 0), preserving add_table(replace=True) semantics for the first call without leaking resources registered by sessions that have since ended. - add_table()/add_tables()/remove_table() are blocked only while a session is actually active, rather than forever after the first server() call. Fails safe: a missed on_ended degrades to over-conservative cleanup skipping, never premature cleanup.
cpsievert
force-pushed
the
fix/py-server-cleanup-and-description-snapshot
branch
from
September 12, 2026 16:57
a097d23 to
701a5ee
Compare
Address Copilot review on #308: - Retain sources/executors replaced while sessions are live and clean them up when the last live session ends (or cleanup() runs), fixing a leaked executor/connection when cleanup_replaced was False. - Correct add_tables() docstring: error occurs while a server session is active, not after .server() has been invoked. - Rename _untrack_session to untrack_session per nested-function naming convention in private modules.
QueryChat(None, table_name=...) stored the name unvalidated, so a bad name was baked into the module id and only failed later, at .server() registration time. Validate with the same rule used at registration.
cpsievert
added a commit
that referenced
this pull request
Sep 12, 2026
…deferred name validation Address Copilot review on #306: - Retain sources/executors replaced while sessions are live (.retired_resources) and clean them up when the last live session ends (or $cleanup() runs), fixing a leaked executor/connection when cleanup_replaced was FALSE. Mirrors the Python side (#308). - greeter$build_client() distinguishes an omitted data_sources/ data_description override (falls back to live state) from an explicit snapshot, which may itself be NULL -- a session whose snapshot had no inferred description no longer picks up a later session's. - QueryChat$new(NULL, table_name = ...) validates the deferred name at construction instead of failing later at $server() registration.
cpsievert
deleted the
fix/py-server-cleanup-and-description-snapshot
branch
September 12, 2026 17:21
3 tasks
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.
The problem
Follow-up to the merged per-session registration stack (#301–#304):
server(data_source=)passescleanup_replaced=Falseunconditionally. Before that stack, the call went throughadd_table(replace=True), which cleaned up the source it replaced. So if the object was constructed with a data source (oradd_table()was called before any session) and this is the first.server()call, the pre-existing source/executor used to be cleaned up and now silently leaks — even though no earlier session can possibly be using it yet.The fix
Track live sessions on the instance (
_active_sessions, incremented when a session's server starts, decremented viasession.on_ended()) and clean up the replaced source/executor exactly when no session is live at registration time (cleanup_replaced = _active_sessions == 0). A registration made while a session is running skips cleanup, since the replaced resource may still be in use by it.Scoping shared-state decisions to live sessions (rather than to whether
.server()has ever been called) also means:add_table()/add_tables()/remove_table()are blocked only while a session is actually active, not forever after the first.server()call.If
on_endednever fires, the count stays elevated and behavior degrades to over-conservative (skipping cleanups), never premature cleanup.What intentionally isn't changed
The other resource-lifecycle point from the Copilot review of the R port (#306) — the greeting snapshot omitting
data_description— doesn't apply to Python:_data_descriptionis fixed at construction and never mutated (nothing assigns to it after__init__), and inferred descriptions live on theDataSourceobjects themselves (get_data_description()), which the greeting snapshot already covers via itsdata_sourcesoverride. The R side needed the fix because itsauto_fill_data_description()mutates shared state at registration time.Test plan
TestServerDataSourceSessionLifecycle): a replaced source is cleaned up once the session that registered it has ended; a replaced source survives while any session is live (overlap protection);add_table()is allowed once all sessions have ended.add_table(replace=True)still does; first-call cleanup of constructor-registered sources) all pass, locking in the concurrent-session behavior.Part of #300.
Review follow-ups (Copilot)
_retired_resources) and cleaned up when the last live session ends (orcleanup()runs) — closing the leaked-executor/connection path flagged in review.QueryChat(None, table_name=...)now validates the deferred name at construction instead of failing later at.server()registration (mirrors the R-side finding on fix(r): let $server(data_source=) survive a second session safely #306).