fix(r): let $server(data_source=) survive a second session safely

[cpsievert]

Stacked on #305

Depends on #305 (deferred-construction ergonomics) — review that first. Two notes on top of the original change:

• fix(r): make table_name optional for deferred QueryChat$new(NULL) #305 now fixes the module-ID desync by removing $add_table()'s single-table id auto-rename entirely (matching Python's set-id-once semantics), rather than pinning the id — so there's no .id_pinned guard to carry into add_or_replace_table() here; the rename block is simply gone.
• greeter$tables is now deduplicated, so per-session registration doesn't append the same table name once per session.

The problem

Some deployments can only create a table's data source inside the Shiny server function — for example, on Posit Connect, a per-user database connection depends on that user's session-scoped OAuth credentials, which don't exist until the server function runs. querychat's R package already supports this "deferred" pattern: construct QueryChat$new(NULL, table_name = ...), then register the real data source per session via $server(data_source = ...).

In practice this only ever worked for the first session:

qc <- QueryChat$new(NULL, table_name = "orders")

server <- function(input, output, session) {
  conn <- get_per_user_connection(session)  # depends on this session's OAuth token
  qc$server(data_source = conn)
}

• A second browser tab, page refresh, or second concurrent user hit "Cannot add tables after server initialization." — $server(data_source=) calls the public $add_table(), which refuses to run once a session has started.
• Even bypassing that guard, $server(data_source=) registers under replace = TRUE, and replacing a table cleans up the one it replaces — closing a database connection, disposing a cached query executor — even though an earlier, still-running session's own chat may still be querying through it.
• The auto-generated welcome greeting is built lazily/asynchronously from live, mutable state (private$.data_sources, greeter$tables, private$.data_description), so a later session's registration could leak into an earlier session's greeting.

This mirrors the equivalent bug already fixed in Python (#302, #303, #304); see #300 for the original writeup covering both languages.

The fix

• $add_table()'s guard-free core (name validation, staging the table, rebuilding the system prompt/executor cache, greeting inclusion) is extracted into a private add_or_replace_table(). $add_table() keeps its guard and calls into that core; $server(data_source=...) calls the core directly, bypassing the guard for this path only.
• Live sessions are tracked on the instance (.active_sessions, incremented per $server() call, decremented via session$onSessionEnded()), and shared-state decisions key off it:
  • add_or_replace_table() gains cleanup_replaced = TRUE; $server()'s internal call passes cleanup_replaced = private$.active_sessions == 0, so a replaced resource (and the cached query executor) is cleaned up exactly when no running session could still be using it. The public $add_table(replace = TRUE) is unaffected — that path still has exactly one owner.
  • The $add_table()/$add_tables()/$remove_table() guards block only while a session is actually live, rather than forever after the first $server() call — so a resource registered by a session that has since ended is reclaimed when a later session replaces it.
  • Fails safe: if onSessionEnded never fires, behavior degrades to over-conservative cleanup skipping, never premature cleanup.
• $server() captures greeting_tables and greeting_data_description snapshots synchronously at call time and threads them (along with the already-per-session data_sources snapshot mod_server() already receives) through to greeter$build_client(), so the lazily-invoked greeting is built from point-in-time snapshots instead of reading live, mutable state whenever the greeting callback actually runs.

Matches the Python side, #308.

Test plan

• New tests in test-server_data_source.R: a second session's $server(data_source=) call succeeds without raising; the public $add_table() guard is enforced while a session is live; a second session's call does not clean up the first session's data source or cached query executor (while the public add_table(replace=TRUE) path still does, locking in unchanged behavior); $server() passes greeting_tables/greeting_data_description snapshots to mod_server(), which forwards them (with the per-session data_sources) to greeter$build_client(); per-session registration does not duplicate greeter$tables; mixing config-time $add_table() with per-session $server(data_source=) behaves as documented (unnamed registration replaces the config-time table (cleaned up when no session is live, left alone while one is), an explicit table_name adds alongside it, and the registry is shared across sessions).
• New session-lifecycle tests: a replaced source is cleaned up once the session that registered it has ended; a replaced source survives while any session is live; $add_table() is allowed once all sessions have ended.
• air format --check is clean and the full test suite passes locally (2123 tests), including fix(r): make table_name optional for deferred QueryChat$new(NULL) #305's deferred-construction tests.

Review follow-ups (Copilot)

• Resources replaced while sessions are live are now retained (.retired_resources) and cleaned up when the last live session ends (or $cleanup() runs) — closing the executor/connection leak in the cleanup_replaced = FALSE path. Matches the Python side (fix(py): scope server(data_source=) cleanup and table guards to live sessions #308).
• greeter$build_client() distinguishes an omitted data_sources/data_description override (live fallback) from an explicit snapshot, which may itself be NULL — a session whose snapshot had no inferred description can't pick up a later session's.
• QueryChat$new(NULL, table_name = ...) validates the deferred name at construction instead of failing later at $server() registration.

[Copilot]

🟡 Changes recommended

Unresolved resource cleanup, session rollback, shared-resource, and snapshot issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

pkg-r/R/QueryChat.R:1277

• If cleanup of the current executor or any current data source throws, control flow exits before this new flush_retired_resources() call. That means $cleanup() can leave the resources retained from live-session replacements open, despite this being the fallback cleanup path. Register the retired-resource flush with on.exit() (or make the current-resource cleanup best-effort) so it runs even when another cleanup fails.

      for (source in private$.data_sources) {
        source$cleanup()
      }
      private$flush_retired_resources()

pkg-r/R/QueryChat.R:1198

• The active-session count is incremented before build_query_executor(), client resolution, history validation, and mod_server() can finish. If any of those operations errors and the caller catches it while the Shiny session remains alive, no rollback runs: later table mutations stay blocked and replacements conservatively skip cleanup indefinitely. Register the count only after setup succeeds, or add an on.exit() rollback until the server setup is complete.

      private$.active_sessions <- private$.active_sessions + 1L
      session$onSessionEnded(function() {

pkg-r/R/QueryChat.R:195

• normalize_data_source() has already created an owned DataFrameSource/DBISource by this point, but this compatibility check is outside the tryCatch that cleans a staged source. A per-session registration that mixes source types therefore throws while leaking the new connection. Include normalization/compatibility/staging in the cleanup-on-error block, or explicitly clean normalized before rethrowing.

      check_source_compatibility(other_sources, normalized, table_name)

pkg-r/R/QueryChatGreeter.R:16

• The updated client_factory contract still omits the new data_description argument even though build_client() forwards it and the implementation requires it. This leaves the internal API documentation inconsistent with the actual callback signature; please include the parameter here.

    #' @param client_factory function(tables, prompt, base, data_sources) returning a configured greeting client.

pkg-r/R/querychat_module.R:57

• These overrides are still lazy promises: $server() passes self$greeter$tables and private$.data_description as expressions, while mod_server() only reads them inside the later greeting_arg callback. A later session can therefore mutate both before the greeting is built, so the advertised point-in-time snapshot is not actually captured. Force these arguments at mod_server() entry (or assign forced locals in $server()) before registering moduleServer.

  greeting_tables = NULL,
  greeting_data_description = NULL

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

[cpsievert]

Copilot review feedback addressed in 9cde4cd:

1. Shared underlying DBI connection disconnected on flush — new DBISource$get_connection() plus a shares_underlying_connection() guard: when the replaced and replacing sources wrap the same connection (two sessions each passing the same raw DBIConnection get their own wrappers), the replacement inherits ownership and the old wrapper is neither cleaned nor retired. Applied to both the $server(data_source=) retirement path and $add_table(replace=TRUE). Covered by new tests (same-conn replacement survives flush; different-conn replacement still gets disconnected; $add_table(replace=TRUE) keeps a shared connection open).
2. $cleanup() could skip the retired-resource flush on error — the flush is now registered with on.exit(), so it runs even if cleaning the current executor or a source throws. Covered by a new test.
3. Session counted before setup completed — $server() now increments .active_sessions (and arms the onSessionEnded flush) only after mod_server() returns successfully, so a failed call on a still-live session no longer blocks table mutations or defers replacement cleanup indefinitely. Covered by a new test (failed $server() leaves $add_table() unblocked).
4. Compatibility check leaked the staged source — check_source_compatibility() moved inside the cleanup-on-error tryCatch in add_or_replace_table(), so a failed per-session registration cleans up the staged source's connection. Covered by a new test asserting the connection is disconnected.
5. client_factory contract doc missing data_description — fixed in QueryChatGreeter.
6. Lazy greeting snapshot args in mod_server() — greeting_tables/greeting_data_description are now force()d at module entry, so the deferred greeting callback sees the session's point-in-time snapshot, not a later session's mutation. Covered by a regression test that mutates the promised bindings between setup and greeting time (verified to fail without the fix).

Note: (1) slightly changes $add_table(replace=TRUE) semantics beyond this PR's scope — replacing a table with the same connection no longer disconnects it (previously it did, breaking the replacement source).

Full pkg-r test suite passes.

[Copilot]

🟡 Changes recommended

Resource ownership, cleanup retry, and registration atomicity issues remain unresolved in QueryChat.R.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (4)

• pkg-r/man/DBISource.Rd: Generated file
• pkg-r/man/DataFrameSource.Rd: Generated file
• pkg-r/man/PinSource.Rd: Generated file
• pkg-r/man/TblSqlSource.Rd: Generated file

Suppressed comments (2)

pkg-r/R/QueryChat.R:263

• Clearing .retired_resources before cleanup and suppressing errors permanently drops any resource whose cleanup fails. A transient DB/engine cleanup error can therefore leave the connection or executor unreachable, and later $cleanup() calls cannot retry it; retain failed entries while continuing through the remaining resources.

      private$.retired_resources <- list()
      for (resource in retired) {
        # Best-effort: one failing cleanup must not leave the rest open.
        tryCatch(resource$cleanup(), error = function(e) NULL)

pkg-r/R/QueryChat.R:199

• Because auto_fill_data_description() mutates .data_description and its mode, calling it before check_source_compatibility() makes a failed per-session registration non-atomic. For example, replacing a single inferred PinSource with an incompatible data-frame source clears the existing inferred description even though the old source remains registered. Validate before mutating and restore both fields on any staging error.

      private$auto_fill_data_description(next_sources)

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