fix(py): don't clean up a table replaced via server(data_source=)

[cpsievert]

Stacked on #302

Depends on #302 (let server(data_source=) survive a second session) — review that first. This PR's diff is just the cleanup-safety fix on top of it.

The problem

Once #302 lets a second session register its own table under the same name, that registration is implemented the same way add_table(replace=True) always has been: "replacing" a table cleans up the one it replaces — closing a database connection, disposing a SQLAlchemy engine, etc.

That's correct when you deliberately reconfigure a table before anyone's connected — the old value really is dead. It's wrong once two sessions can register concurrently: a second session's own resource is unrelated to the first session's, but registering it under the same table name still triggers cleanup of the first session's resource. Concretely, with the deferred per-session pattern:

qc = QueryChat(None, table_name="orders")

def server(input, output, session):
    conn = get_per_user_connection(session)  # e.g. a per-user SQLAlchemy engine
    qc.server(data_source=conn)

• Session A registers conn_A. Fine.
• Session B registers conn_B under the same "orders" name. This closes/disposes conn_A — session A's own connection — even though session A is a completely unrelated user who is still actively using it. Session A's next query then fails.

The fix

_add_or_replace_table() gains cleanup_replaced=False, used only by server(data_source=...)'s internal call. A table replaced through that path is left alone; cleaning it up becomes the responsibility of whoever created it (e.g. closing it themselves via the framework's own session-end hook), since another session may still depend on it.

The public add_table(replace=True) is unaffected — it still cleans up the table it replaces, since that path has exactly one owner.

Test plan

• New test: a second session's server(data_source=...) call does not call .cleanup() on the first session's data source.
• New test: the public add_table(replace=True) still calls .cleanup() on the table it replaces (locks in unchanged behavior).

[Copilot]

🟡 Changes recommended

Unresolved critical executor-lifetime issues and a moderate source cleanup ownership issue remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR prevents server(data_source=...) from prematurely cleaning up resources used by other sessions.

Changes:

• Adds conditional cleanup for replaced data sources.
• Preserves cleanup for public add_table(replace=True).
• Adds regression tests and changelog documentation.

File summaries

File	Summary	Findings
pkg-py/tests/test_server_data_source.py	Adds replacement cleanup regression tests.	None
pkg-py/src/querychat/_shiny.py	Disables cleanup for per-session replacements.	Critical executor-lifetime issue (2 votes); moderate source cleanup ownership issue (1 vote).
pkg-py/src/querychat/_querychat_base.py	Implements conditional replacement cleanup.	Critical executor cleanup issue (1 vote).
pkg-py/CHANGELOG.md	Documents the behavior change.	None

Review details

Suppressed comments (2)

pkg-py/src/querychat/_querychat_base.py:564

• For a raw pandas/Polars DataFrame, normalize_data_source() creates a DataFrameSource with its own DuckDB connection. With cleanup_replaced=False, the old normalized source is dropped from _data_sources, and the later qc.cleanup() only visits the current source; this path also registers no session-end cleanup for the replaced source. Repeated sessions can therefore leak one connection per replacement, even though the caller only supplied the original frame and cannot normally clean the wrapper. Please retain these sources for lifecycle cleanup or add an explicit session-scoped cleanup mechanism.

        if cleanup_replaced and old_source is not None and old_source is not normalized:
            old_source.cleanup()

pkg-py/src/querychat/_shiny.py:719

• This avoids cross-session teardown, but it also drops the previous normalized source from self._data_sources without registering any replacement owner. For inputs such as a pandas DataFrame, _add_or_replace_table() creates a DataFrameSource (and its DuckDB connection); after the next session replaces it, QueryChat.cleanup() only visits the latest source, so the earlier connection is never deterministically closed. Track the per-session source and clean it at that session's end, or otherwise retain and close all sources created by this path with clear ownership semantics.

                cleanup_replaced=False,

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Retired normalized sources and executors can leak because their session ownership and cleanup lifecycle are not retained.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

pkg-py/src/querychat/_querychat_base.py:564

• Skipping old_source.cleanup() here can leak resources that QueryChat created during normalization. For a raw DataFrame, normalize_data_source() creates a DataFrameSource with its own DuckDB connection, so the caller only owns the original frame and cannot clean up this wrapper after it is removed from _data_sources; qc.cleanup() only visits the current sources. Keep the per-session normalized source associated with its session and close it from that session's end hook (after the session no longer uses it), rather than dropping the only framework-owned reference.

        if cleanup_replaced and old_source is not None and old_source is not normalized:
            old_source.cleanup()

pkg-py/src/querychat/_shiny.py:719

• Disabling replacement cleanup here also removes the previous normalized source from self._data_sources, but this path does not register any session-end cleanup or retain retired resources for QueryChat.cleanup(). For an ordinary pandas input, normalize_data_source() creates a DataFrameSource with its own DuckDB connection, so after session B replaces session A, qc.cleanup() can only close B's source and A's connection is left to nondeterministic garbage collection. Please add explicit retired-source/session ownership tracking (or an equivalent lifecycle hook) so repeated sessions do not leak resources.

                cleanup_replaced=False,

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite