From 676df17f06e2817d74e2f871c58ba43a31a1699e Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 28 Aug 2026 16:29:20 -0500 Subject: [PATCH 01/11] Add commons_prewarm() for idle-time pre-warming commons_server() used to kick off pre-warming itself; with it gone, export a helper so custom apps get the same behavior with one call. commons_prewarm(agent) validates the agent and defers prewarm() to post-startup idle time, and is used by commons_app() and throughout the examples, vignette, and onboarding skill. The error contract is split by call site. A direct agent$prewarm() is typically warming caches ahead of deployment, so failures propagate: a cold cache should fail the deploy, and a warning would sail through a deploy script. commons_prewarm() downgrades failures to warnings, since pre-warming is a pure optimization (everything it builds is rebuilt lazily at first use) and an error escaping a later::later() callback would stop the Shiny app. --- pkg-r/NAMESPACE | 1 + pkg-r/R/chat.R | 58 +++++++++++++++++++++++++---- pkg-r/R/commons.R | 6 +++ pkg-r/R/data-source.R | 15 ++++---- pkg-r/man/commons_prewarm.Rd | 46 +++++++++++++++++++++++ pkg-r/man/data_source.Rd | 15 ++++---- pkg-r/tests/testthat/test-chat.R | 30 +++++++++++++++ pkg-r/tests/testthat/test-commons.R | 12 ++++++ 8 files changed, 161 insertions(+), 22 deletions(-) create mode 100644 pkg-r/man/commons_prewarm.Rd diff --git a/pkg-r/NAMESPACE b/pkg-r/NAMESPACE index c9004cba..94ac6310 100644 --- a/pkg-r/NAMESPACE +++ b/pkg-r/NAMESPACE @@ -2,6 +2,7 @@ export(commons) export(commons_app) +export(commons_prewarm) export(commons_server) export(commons_theme) export(context_layer) diff --git a/pkg-r/R/chat.R b/pkg-r/R/chat.R index 7d1bba23..76386c11 100644 --- a/pkg-r/R/chat.R +++ b/pkg-r/R/chat.R @@ -111,14 +111,7 @@ commons_server <- function(id, client, ...) { attributes = list("commons.server.id" = id) ) - # Build the context index and start the background pin-cache download - # during post-startup idle time (while the user reads the welcome message). - # Errors are swallowed: the first search retries the index build and - # surfaces the failure to the model, and an unwarmed pin is simply - # downloaded at its first use. - later::later(function() { - tryCatch(client$prewarm(), error = function(err) NULL) - }) + commons_prewarm(client) chat <- shinychat::chat_server(id, client = client, ...) # shinychat owns the conversation identity (it sets the client's @@ -130,6 +123,55 @@ commons_server <- function(id, client, ...) { chat } +#' Pre-warm a commons agent during post-startup idle time +#' +#' A [commons()] agent builds its context index on first use. To move that +#' cost off the first question, call `commons_prewarm()` in a Shiny server +#' function: it defers the agent's `prewarm()` method to post-startup idle +#' time, so the index builds while the user reads the welcome message. +#' +#' `prewarm()` is synchronous and independent of the Shiny runtime, so it can +#' also be called directly to warm the on-disk cache ahead of deployment. It +#' also starts a background process that downloads any uncached pins into the +#' local pins cache (see [data_source()]). +#' +#' `prewarm()` lets failures propagate, since a direct call is typically +#' warming caches ahead of deployment and a mere warning would sail through +#' a deploy script. `commons_prewarm()` downgrades such failures to +#' warnings: pre-warming is a pure optimization — everything it builds is +#' rebuilt lazily at first use — and an error escaping the [later::later()] +#' callback would stop the app. +#' +#' @param client A [commons()] agent. +#' +#' @return `NULL`, invisibly. +#' +#' @examples +#' \dontrun{ +#' server <- function(input, output, session) { +#' agent <- commons( +#' ellmer::chat_anthropic(), +#' data_sources = data_source(sales = sales) +#' ) +#' commons_prewarm(agent) +#' shinychat::chat_server("chat", client = agent) +#' } +#' } +#' +#' @export +commons_prewarm <- function(client) { + check_commons_client(client) + # An error escaping a later::later() callback stops the Shiny app, and + # pre-warming is a pure optimization, so downgrade failures to warnings. + later::later(function() { + tryCatch( + client$prewarm(), + error = function(err) cli::cli_warn(conditionMessage(err)) + ) + }) + invisible(NULL) +} + check_chat_packages <- function(call = rlang::caller_env()) { missing <- c("htmltools", "shiny", "shinychat")[ !vapply( diff --git a/pkg-r/R/commons.R b/pkg-r/R/commons.R index 92da5671..b0e970e8 100644 --- a/pkg-r/R/commons.R +++ b/pkg-r/R/commons.R @@ -390,6 +390,12 @@ Commons <- R6::R6Class( }, prewarm = function() { + # Pre-warming is a pure optimization (everything it builds is rebuilt + # or downloaded lazily at first use), but a direct call is typically + # warming caches ahead of deployment, so failures propagate: a cold + # cache should fail the deploy. commons_prewarm() downgrades failures + # to warnings for the Shiny idle-time path, where an escaping error + # would stop the app. layer <- private$context_layer layer_state <- if (is.null(layer)) NULL else context_layer_state(layer) if (!is.null(layer_state) && length(layer_state$docs) > 0) { diff --git a/pkg-r/R/data-source.R b/pkg-r/R/data-source.R index c1437429..6560afd3 100644 --- a/pkg-r/R/data-source.R +++ b/pkg-r/R/data-source.R @@ -12,13 +12,14 @@ #' * A `pins` board, e.g. [pins::board_connect()], is read into the same #' in-process database: each pin in `tables` becomes a table. Pin names are #' validated against the board at construction (a single listing call), but -#' each pin is downloaded only when its table is first used. -#' [commons_server()] starts a background process right after startup that -#' downloads the remaining pins into the local pins cache, so a first use -#' typically only reads an already-downloaded file. A table reflects the pin's -#' value at first use and is not refreshed for the lifetime of the data -#' source; if a pin can't be read (e.g. a network failure), the error surfaces -#' at that first use and the read is retried on the next one. +#' each pin is downloaded only when its table is first used. Calling the +#' agent's `prewarm()` method (see [commons_prewarm()]) starts a background +#' process that downloads the remaining pins into the local pins cache, so +#' a first use typically only reads an already-downloaded file. A table +#' reflects the pin's value at first use and is not refreshed for the +#' lifetime of the data source; if a pin can't be read (e.g. a network +#' failure), the error surfaces at that first use and the read is retried +#' on the next one. #' #' @param ... A single DBI connection, a single `pins` board, or named data #' frames to register as tables. When passing data frames, each name becomes diff --git a/pkg-r/man/commons_prewarm.Rd b/pkg-r/man/commons_prewarm.Rd new file mode 100644 index 00000000..5705371d --- /dev/null +++ b/pkg-r/man/commons_prewarm.Rd @@ -0,0 +1,46 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/chat.R +\name{commons_prewarm} +\alias{commons_prewarm} +\title{Pre-warm a commons agent during post-startup idle time} +\usage{ +commons_prewarm(client) +} +\arguments{ +\item{client}{A \code{\link[=commons]{commons()}} agent.} +} +\value{ +\code{NULL}, invisibly. +} +\description{ +A \code{\link[=commons]{commons()}} agent builds its context index on first use. To move that +cost off the first question, call \code{commons_prewarm()} in a Shiny server +function: it defers the agent's \code{prewarm()} method to post-startup idle +time, so the index builds while the user reads the welcome message. +} +\details{ +\code{prewarm()} is synchronous and independent of the Shiny runtime, so it can +also be called directly to warm the on-disk cache ahead of deployment. It +also starts a background process that downloads any uncached pins into the +local pins cache (see \code{\link[=data_source]{data_source()}}). + +\code{prewarm()} lets failures propagate, since a direct call is typically +warming caches ahead of deployment and a mere warning would sail through +a deploy script. \code{commons_prewarm()} downgrades such failures to +warnings: pre-warming is a pure optimization — everything it builds is +rebuilt lazily at first use — and an error escaping the \code{\link[later:later]{later::later()}} +callback would stop the app. +} +\examples{ +\dontrun{ +server <- function(input, output, session) { + agent <- commons( + ellmer::chat_anthropic(), + data_sources = data_source(sales = sales) + ) + commons_prewarm(agent) + shinychat::chat_server("chat", client = agent) +} +} + +} diff --git a/pkg-r/man/data_source.Rd b/pkg-r/man/data_source.Rd index ad45e24d..c922537d 100644 --- a/pkg-r/man/data_source.Rd +++ b/pkg-r/man/data_source.Rd @@ -61,13 +61,14 @@ when the data isn't already in a database. \item A \code{pins} board, e.g. \code{\link[pins:board_connect]{pins::board_connect()}}, is read into the same in-process database: each pin in \code{tables} becomes a table. Pin names are validated against the board at construction (a single listing call), but -each pin is downloaded only when its table is first used. -\code{\link[=commons_server]{commons_server()}} starts a background process right after startup that -downloads the remaining pins into the local pins cache, so a first use -typically only reads an already-downloaded file. A table reflects the pin's -value at first use and is not refreshed for the lifetime of the data -source; if a pin can't be read (e.g. a network failure), the error surfaces -at that first use and the read is retried on the next one. +each pin is downloaded only when its table is first used. Calling the +agent's \code{prewarm()} method (see \code{\link[=commons_prewarm]{commons_prewarm()}}) starts a background +process that downloads the remaining pins into the local pins cache, so +a first use typically only reads an already-downloaded file. A table +reflects the pin's value at first use and is not refreshed for the +lifetime of the data source; if a pin can't be read (e.g. a network +failure), the error surfaces at that first use and the read is retried +on the next one. } } \section{Data dictionaries}{ diff --git a/pkg-r/tests/testthat/test-chat.R b/pkg-r/tests/testthat/test-chat.R index 8d67dcab..dc0845d4 100644 --- a/pkg-r/tests/testthat/test-chat.R +++ b/pkg-r/tests/testthat/test-chat.R @@ -105,3 +105,33 @@ test_that("commons_server requires a commons agent", { error = TRUE ) }) + +test_that("commons_prewarm() downgrades prewarm failures to warnings", { + path <- withr::local_tempfile(fileext = ".md") + writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) + agent <- test_agent(context_layer = context_layer(files = path)) + + local_mocked_bindings( + context_store = function(...) stop("index build exploded"), + .package = "commons" + ) + commons_prewarm(agent) + expect_warning(later::run_now(), "index build exploded") +}) + +test_that("commons_app() prewarms the agent on idle", { + skip_if_not_installed("shiny") + skip_if_not_installed("shinychat") + + app <- commons_app(test_agent()) + app_env <- environment(app$serverFuncSource) + prewarmed <- FALSE + testthat::local_mocked_bindings( + commons_prewarm = function(client) prewarmed <<- TRUE, + .package = "commons" + ) + shiny::testServer(app_env$server, { + session$flushReact() + }) + expect_true(prewarmed) +}) diff --git a/pkg-r/tests/testthat/test-commons.R b/pkg-r/tests/testthat/test-commons.R index fb2e4b6d..5a569a2f 100644 --- a/pkg-r/tests/testthat/test-commons.R +++ b/pkg-r/tests/testthat/test-commons.R @@ -437,6 +437,18 @@ test_that("prewarm() without a context layer is a no-op", { expect_no_error(test_agent()$prewarm()) }) +test_that("prewarm() propagates failures", { + path <- withr::local_tempfile(fileext = ".md") + writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) + agent <- test_agent(context_layer = context_layer(files = path)) + + local_mocked_bindings( + context_store = function(...) stop("index build exploded"), + .package = "commons" + ) + expect_error(agent$prewarm(), "index build exploded") +}) + test_that("prewarm() records a cache-miss build and its own span", { skip_if_not_installed("otelsdk") From 4dc549464106d5a6571ea03a28d4e4e8fd6d33e5 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 28 Aug 2026 14:11:19 -0500 Subject: [PATCH 02/11] Split prewarm() into prewarm_context() and prewarm_sources() The two jobs differ in cost, process model, and persistence: the context index is synchronous, in-process, and in-memory (each session rebuilds its own), while pins warming is a background process filling a shared on-disk cache that can also be warmed offline ahead of deployment. Naming them separately makes call sites self-documenting and lets offline workflows warm only the persistent half. prewarm() remains as both. A persistent context store is noted as a possible future move (posit-dev/commons#214). --- pkg-r/R/chat.R | 25 ++++++++++++++--------- pkg-r/R/commons.R | 10 ++++++++++ pkg-r/R/data-source.R | 12 ++++++----- pkg-r/man/commons_prewarm.Rd | 25 +++++++++++++++-------- pkg-r/man/data_source.Rd | 12 ++++++----- pkg-r/tests/testthat/test-commons.R | 31 ++++++++++++++++++++--------- pkg-r/vignettes/commons.Rmd | 2 +- 7 files changed, 80 insertions(+), 37 deletions(-) diff --git a/pkg-r/R/chat.R b/pkg-r/R/chat.R index 76386c11..f78d6b1d 100644 --- a/pkg-r/R/chat.R +++ b/pkg-r/R/chat.R @@ -125,15 +125,22 @@ commons_server <- function(id, client, ...) { #' Pre-warm a commons agent during post-startup idle time #' -#' A [commons()] agent builds its context index on first use. To move that -#' cost off the first question, call `commons_prewarm()` in a Shiny server -#' function: it defers the agent's `prewarm()` method to post-startup idle -#' time, so the index builds while the user reads the welcome message. -#' -#' `prewarm()` is synchronous and independent of the Shiny runtime, so it can -#' also be called directly to warm the on-disk cache ahead of deployment. It -#' also starts a background process that downloads any uncached pins into the -#' local pins cache (see [data_source()]). +#' A [commons()] agent defers two kinds of setup to first use, and exposes a +#' `prewarm()` method for each so you can move the cost off the first +#' question: +#' +#' * `agent$prewarm_context()` builds the context index (the store behind +#' `search_context`). It is synchronous and in-process: the index is +#' in-memory, so each Shiny session's agent builds its own. +#' * `agent$prewarm_sources()` starts a background process that downloads +#' any uncached pins into the local pins cache (see [data_source()]). +#' Because the pins cache is on disk, this can also run ahead of +#' deployment — outside the Shiny runtime entirely — and the deployed +#' app reads the warmed cache. +#' +#' `agent$prewarm()` calls both. Call `commons_prewarm()` in a Shiny server +#' function to defer warming to post-startup idle time, so it happens while +#' the user reads the welcome message. #' #' `prewarm()` lets failures propagate, since a direct call is typically #' warming caches ahead of deployment and a mere warning would sail through diff --git a/pkg-r/R/commons.R b/pkg-r/R/commons.R index b0e970e8..2c20d07d 100644 --- a/pkg-r/R/commons.R +++ b/pkg-r/R/commons.R @@ -396,6 +396,12 @@ Commons <- R6::R6Class( # cache should fail the deploy. commons_prewarm() downgrades failures # to warnings for the Shiny idle-time path, where an escaping error # would stop the app. + self$prewarm_context() + self$prewarm_sources() + invisible(self) + }, + + prewarm_context = function() { layer <- private$context_layer layer_state <- if (is.null(layer)) NULL else context_layer_state(layer) if (!is.null(layer_state) && length(layer_state$docs) > 0) { @@ -408,6 +414,10 @@ Commons <- R6::R6Class( ) context_store(layer) } + invisible(self) + }, + + prewarm_sources = function() { for (source in private$sources) { source_prewarm(source) } diff --git a/pkg-r/R/data-source.R b/pkg-r/R/data-source.R index 6560afd3..af140652 100644 --- a/pkg-r/R/data-source.R +++ b/pkg-r/R/data-source.R @@ -13,11 +13,13 @@ #' in-process database: each pin in `tables` becomes a table. Pin names are #' validated against the board at construction (a single listing call), but #' each pin is downloaded only when its table is first used. Calling the -#' agent's `prewarm()` method (see [commons_prewarm()]) starts a background -#' process that downloads the remaining pins into the local pins cache, so -#' a first use typically only reads an already-downloaded file. A table -#' reflects the pin's value at first use and is not refreshed for the -#' lifetime of the data source; if a pin can't be read (e.g. a network +#' agent's `prewarm_sources()` method (see [commons_prewarm()]) starts a +#' background process that downloads the remaining pins into the local +#' pins cache, so a first use typically only reads an already-downloaded +#' file. Since the pins cache is on disk, `prewarm_sources()` can also run +#' ahead of deployment to warm the cache the deployed app will read. A +#' table reflects the pin's value at first use and is not refreshed for +#' the lifetime of the data source; if a pin can't be read (e.g. a network #' failure), the error surfaces at that first use and the read is retried #' on the next one. #' diff --git a/pkg-r/man/commons_prewarm.Rd b/pkg-r/man/commons_prewarm.Rd index 5705371d..6d21d878 100644 --- a/pkg-r/man/commons_prewarm.Rd +++ b/pkg-r/man/commons_prewarm.Rd @@ -13,16 +13,25 @@ commons_prewarm(client) \code{NULL}, invisibly. } \description{ -A \code{\link[=commons]{commons()}} agent builds its context index on first use. To move that -cost off the first question, call \code{commons_prewarm()} in a Shiny server -function: it defers the agent's \code{prewarm()} method to post-startup idle -time, so the index builds while the user reads the welcome message. +A \code{\link[=commons]{commons()}} agent defers two kinds of setup to first use, and exposes a +\code{prewarm()} method for each so you can move the cost off the first +question: } \details{ -\code{prewarm()} is synchronous and independent of the Shiny runtime, so it can -also be called directly to warm the on-disk cache ahead of deployment. It -also starts a background process that downloads any uncached pins into the -local pins cache (see \code{\link[=data_source]{data_source()}}). +\itemize{ +\item \code{agent$prewarm_context()} builds the context index (the store behind +\code{search_context}). It is synchronous and in-process: the index is +in-memory, so each Shiny session's agent builds its own. +\item \code{agent$prewarm_sources()} starts a background process that downloads +any uncached pins into the local pins cache (see \code{\link[=data_source]{data_source()}}). +Because the pins cache is on disk, this can also run ahead of +deployment — outside the Shiny runtime entirely — and the deployed +app reads the warmed cache. +} + +\code{agent$prewarm()} calls both. Call \code{commons_prewarm()} in a Shiny server +function to defer warming to post-startup idle time, so it happens while +the user reads the welcome message. \code{prewarm()} lets failures propagate, since a direct call is typically warming caches ahead of deployment and a mere warning would sail through diff --git a/pkg-r/man/data_source.Rd b/pkg-r/man/data_source.Rd index c922537d..390bfc6e 100644 --- a/pkg-r/man/data_source.Rd +++ b/pkg-r/man/data_source.Rd @@ -62,11 +62,13 @@ when the data isn't already in a database. in-process database: each pin in \code{tables} becomes a table. Pin names are validated against the board at construction (a single listing call), but each pin is downloaded only when its table is first used. Calling the -agent's \code{prewarm()} method (see \code{\link[=commons_prewarm]{commons_prewarm()}}) starts a background -process that downloads the remaining pins into the local pins cache, so -a first use typically only reads an already-downloaded file. A table -reflects the pin's value at first use and is not refreshed for the -lifetime of the data source; if a pin can't be read (e.g. a network +agent's \code{prewarm_sources()} method (see \code{\link[=commons_prewarm]{commons_prewarm()}}) starts a +background process that downloads the remaining pins into the local +pins cache, so a first use typically only reads an already-downloaded +file. Since the pins cache is on disk, \code{prewarm_sources()} can also run +ahead of deployment to warm the cache the deployed app will read. A +table reflects the pin's value at first use and is not refreshed for +the lifetime of the data source; if a pin can't be read (e.g. a network failure), the error surfaces at that first use and the read is retried on the next one. } diff --git a/pkg-r/tests/testthat/test-commons.R b/pkg-r/tests/testthat/test-commons.R index 5a569a2f..d692a3b6 100644 --- a/pkg-r/tests/testthat/test-commons.R +++ b/pkg-r/tests/testthat/test-commons.R @@ -418,7 +418,7 @@ test_that("commons() errors on injection parameters matching no name", { }) -test_that("prewarm() builds the context store ahead of the first search", { +test_that("prewarm_context() builds the context store ahead of the first search", { path <- withr::local_tempfile(fileext = ".md") writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) layer <- context_layer(files = path) @@ -428,13 +428,25 @@ test_that("prewarm() builds the context store ahead of the first search", { agent <- test_agent(context_layer = layer) expect_null(context_layer_state(layer)$store) - agent$prewarm() + agent$prewarm_context() expect_false(is.null(context_layer_state(layer)$store)) expect_match(context_search(layer, "revenue")[[1]], "booked") }) +test_that("prewarm() warms both context and sources", { + path <- withr::local_tempfile(fileext = ".md") + writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) + layer <- context_layer(files = path) + agent <- test_agent(context_layer = layer) + + agent$prewarm() + expect_false(is.null(context_layer_state(layer)$store)) +}) + test_that("prewarm() without a context layer is a no-op", { expect_no_error(test_agent()$prewarm()) + expect_no_error(test_agent()$prewarm_context()) + expect_no_error(test_agent()$prewarm_sources()) }) test_that("prewarm() propagates failures", { @@ -447,16 +459,17 @@ test_that("prewarm() propagates failures", { .package = "commons" ) expect_error(agent$prewarm(), "index build exploded") + expect_error(agent$prewarm_context(), "index build exploded") }) -test_that("prewarm() records a cache-miss build and its own span", { +test_that("prewarm_context() records a cache-miss build and its own span", { skip_if_not_installed("otelsdk") path <- withr::local_tempfile(fileext = ".md") writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) agent <- test_agent(context_layer = context_layer(files = path)) - recorded <- otelsdk::with_otel_record(agent$prewarm()) + recorded <- otelsdk::with_otel_record(agent$prewarm_context()) names <- vapply(recorded$traces, `[[`, character(1), "name") expect_true("commons_context_store_build" %in% names) @@ -468,15 +481,15 @@ test_that("prewarm() records a cache-miss build and its own span", { expect_equal(prewarm_span$attributes[["commons.context.cache_hit"]], FALSE) }) -test_that("prewarm() records a cache hit without a build span", { +test_that("prewarm_context() records a cache hit without a build span", { skip_if_not_installed("otelsdk") path <- withr::local_tempfile(fileext = ".md") writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) agent <- test_agent(context_layer = context_layer(files = path)) - agent$prewarm() + agent$prewarm_context() - recorded <- otelsdk::with_otel_record(agent$prewarm()) + recorded <- otelsdk::with_otel_record(agent$prewarm_context()) names <- vapply(recorded$traces, `[[`, character(1), "name") expect_false("commons_context_store_build" %in% names) @@ -484,7 +497,7 @@ test_that("prewarm() records a cache hit without a build span", { expect_equal(prewarm_span$attributes[["commons.context.cache_hit"]], TRUE) }) -test_that("prewarm() warms board pins in the background without loading them", { +test_that("prewarm_sources() warms board pins in the background without loading them", { skip_if_not_installed("pins") board <- board_with_pins( @@ -499,7 +512,7 @@ test_that("prewarm() warms board pins in the background without loading them", { expect_length(DBI::dbListTables(data_source_state(src)$con), 0) - agent$prewarm() + agent$prewarm_sources() p <- data_source_state(src)$pending$process expect_s3_class(p, "r_process") withr::defer(p$kill()) diff --git a/pkg-r/vignettes/commons.Rmd b/pkg-r/vignettes/commons.Rmd index 56bf6e2f..55016e40 100644 --- a/pkg-r/vignettes/commons.Rmd +++ b/pkg-r/vignettes/commons.Rmd @@ -318,4 +318,4 @@ agent <- commons( commons_app(agent) ``` -Use `commons_app()` to run the agent in a local or single-user Shiny app. For multi-user deployments, compose shinychat's UI with `commons_theme()` on the page and `commons_server()` in the server, and create a new agent for each Shiny session. This example assumes that `observations` and `site_area` are data frames loaded when the app starts. +Use `commons_app()` to run the agent in a local or single-user Shiny app. For multi-user deployments, compose shinychat's UI with `commons_theme()` on the page and `commons_server()` in the server, and create a new agent for each Shiny session. `commons_server()` warms the agent during post-startup idle time (see `commons_prewarm()`); `agent$prewarm_context()` and `agent$prewarm_sources()` warm the context index and pins cache individually. This example assumes that `observations` and `site_area` are data frames loaded when the app starts. From d58cd40a52d6d2a6f2824018de7e6278af2695a3 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 28 Aug 2026 14:24:26 -0500 Subject: [PATCH 03/11] Make the context store persistent and content-addressed The store behind search_context was an in-memory ragnar store rebuilt from scratch by every process. It is now a DuckDB file keyed by a hash of the layer's docs (salted with ragnar/duckdb versions), so a build happens once per content version per cache root and every later session opens it read-only in milliseconds. Cold builds write a temp file and rename it into place atomically, so concurrent builders never expose a partial store; new content is a new key, so there is no invalidation logic. The cache root resolves from the commons.context_cache option, the COMMONS_CONTEXT_CACHE or CONNECT_CONTENT_DATA_DIR environment variables (Connect's early-access persistent data directories survive deployments), or the per-user cache dir. prewarm_context() now means 'ensure the store for this content exists' and can run offline, in CI, or at deploy time. Also close a race on the pins path: the background prewarm downloader and a first-use pin_read() could write the same cache entry concurrently (pins has no cache locking), risking a truncated entry that poisons later reads. Both sides now take an exclusive filelock keyed by cache path and pin name. Closes #214 --- pkg-r/DESCRIPTION | 1 + pkg-r/R/chat.R | 8 +- pkg-r/R/commons.R | 4 +- pkg-r/R/context-layer.R | 88 ++++++++++++++++++---- pkg-r/R/data-source.R | 23 +++++- pkg-r/man/commons_prewarm.Rd | 8 +- pkg-r/tests/testthat/setup-context-cache.R | 7 ++ pkg-r/tests/testthat/test-commons.R | 3 + pkg-r/tests/testthat/test-context-layer.R | 21 ++++++ 9 files changed, 141 insertions(+), 22 deletions(-) create mode 100644 pkg-r/tests/testthat/setup-context-cache.R diff --git a/pkg-r/DESCRIPTION b/pkg-r/DESCRIPTION index 9b770cbf..ea520960 100644 --- a/pkg-r/DESCRIPTION +++ b/pkg-r/DESCRIPTION @@ -27,6 +27,7 @@ Imports: duckdb (>= 1.5.4.2), ellmer (>= 0.4.1), evaluate, + filelock, glue, highr, htmltools, diff --git a/pkg-r/R/chat.R b/pkg-r/R/chat.R index f78d6b1d..a98f4bd7 100644 --- a/pkg-r/R/chat.R +++ b/pkg-r/R/chat.R @@ -130,8 +130,12 @@ commons_server <- function(id, client, ...) { #' question: #' #' * `agent$prewarm_context()` builds the context index (the store behind -#' `search_context`). It is synchronous and in-process: the index is -#' in-memory, so each Shiny session's agent builds its own. +#' `search_context`). The index is a persistent, content-addressed file, +#' so the build happens once per content version: later sessions open it +#' in milliseconds, and it can be built offline ahead of deployment. The +#' cache root resolves from the `commons.context_cache` option, the +#' `COMMONS_CONTEXT_CACHE` or `CONNECT_CONTENT_DATA_DIR` environment +#' variables, or the per-user cache directory, in that order. #' * `agent$prewarm_sources()` starts a background process that downloads #' any uncached pins into the local pins cache (see [data_source()]). #' Because the pins cache is on disk, this can also run ahead of diff --git a/pkg-r/R/commons.R b/pkg-r/R/commons.R index 2c20d07d..fbf26dfc 100644 --- a/pkg-r/R/commons.R +++ b/pkg-r/R/commons.R @@ -409,7 +409,9 @@ Commons <- R6::R6Class( "commons_context_prewarm", attributes = list( "commons.context.n_docs" = length(layer_state$docs), - "commons.context.cache_hit" = !is.null(layer_state$store) + "commons.context.cache_hit" = + !is.null(layer_state$store) || + file.exists(context_store_path(layer_state$docs)) ) ) context_store(layer) diff --git a/pkg-r/R/context-layer.R b/pkg-r/R/context-layer.R index 2c352ccd..e1470fe0 100644 --- a/pkg-r/R/context-layer.R +++ b/pkg-r/R/context-layer.R @@ -106,25 +106,83 @@ strip_frontmatter <- function(md) { sub("(?s)^---\r?\n.*?\r?\n---(\r?\n|$)", "", md, perl = TRUE) } -# Store setup (duckdb creation, chunk insertion, FTS indexing) is the most -# expensive part of building an agent and many conversations never search, so -# it's deferred to the first search. Aliases of one layer share its store; -# augmenting its documents creates a layer with a fresh store. +# The context store is a persistent, content-addressed DuckDB file: the key +# hashes the layer's docs plus the ragnar/duckdb versions (whose file format +# the store depends on), so a build happens once per content version per +# cache root and every process afterwards opens it read-only. Store setup is +# still deferred to the first search (or prewarm_context()) since many +# conversations never search, but on a warm cache that first search only +# pays for opening a file. Aliases of one layer share its store; augmenting +# its documents creates a layer with a fresh store. context_store <- function(layer) { state <- context_layer_state(layer) - if (is.null(state$store)) { - local_commons_span( - "commons_context_store_build", - attributes = list("commons.context.n_docs" = length(state$docs)) - ) - store <- ragnar::ragnar_store_create(embed = NULL) - for (doc in state$docs) { - ragnar::ragnar_store_insert(store, ragnar::markdown_chunk(doc)) + if (!is.null(state$store)) { + return(state$store) + } + path <- context_store_path(state$docs) + if (!file.exists(path)) { + build_context_store(state$docs, path) + } + store <- ragnar::ragnar_store_connect(path) + state$store <- store + store +} + +# Build to a temp file in the same directory, then rename into place +# atomically, so a concurrent reader or builder never observes a partial +# store. If another builder wins the race, its store is equivalent content; +# discard ours and open theirs. +build_context_store <- function(docs, path) { + local_commons_span( + "commons_context_store_build", + attributes = list("commons.context.n_docs" = length(docs)) + ) + dir.create(dirname(path), recursive = TRUE, showWarnings = FALSE) + tmp <- tempfile(pattern = ".build-", tmpdir = dirname(path)) + on.exit(unlink(tmp, recursive = TRUE), add = TRUE) + + store <- ragnar::ragnar_store_create(tmp, embed = NULL) + for (doc in docs) { + ragnar::ragnar_store_insert(store, ragnar::markdown_chunk(doc)) + } + ragnar::ragnar_store_build_index(store, type = "fts") + DBI::dbDisconnect(store@con, shutdown = TRUE) + + if (!file.exists(path)) { + file.rename(tmp, path) + } + if (!file.exists(path)) { + cli::cli_abort("Failed to build the context store at {.path {path}}.") + } + invisible(path) +} + +context_store_path <- function(docs) { + key <- rlang::hash(c( + docs, + paste0("ragnar:", utils::packageVersion("ragnar")), + paste0("duckdb:", utils::packageVersion("duckdb")) + )) + file.path(context_cache_dir(), "context", paste0(key, ".duckdb")) +} + +# Cache root resolution: an explicit override, then Connect's persistent +# data directory (survives deployments when the server enables it), then the +# per-user cache dir. Wherever the root is ephemeral (e.g. Connect Cloud, +# which resets disk to the deployed bundle), the store simply rebuilds once +# per cache lifetime instead of once per process. +context_cache_dir <- function() { + opt <- getOption("commons.context_cache") + if (!is.null(opt)) { + return(opt) + } + for (env in c("COMMONS_CONTEXT_CACHE", "CONNECT_CONTENT_DATA_DIR")) { + val <- Sys.getenv(env, unset = NA_character_) + if (!is.na(val) && nzchar(val)) { + return(val) } - ragnar::ragnar_store_build_index(store, type = "fts") - state$store <- store } - state$store + tools::R_user_dir("commons", "cache") } context_search <- function(layer, query, n = 3) { diff --git a/pkg-r/R/data-source.R b/pkg-r/R/data-source.R index af140652..7402098f 100644 --- a/pkg-r/R/data-source.R +++ b/pkg-r/R/data-source.R @@ -458,7 +458,7 @@ source_ensure_tables <- function(source, tables, call = rlang::caller_env()) { for (table in todo) { pin <- pending$pins[[table]] value <- tryCatch( - pins::pin_read(pending$board, pin), + with_pin_lock(pending$board, pin, pins::pin_read(pending$board, pin)), error = function(err) { cli::cli_abort( "Failed to read pin {.val {pin}} for table {.val {table}}.", @@ -487,6 +487,25 @@ source_ensure_all <- function(source, call = rlang::caller_env()) { source_ensure_tables(source, state$tables, call = call) } +# pins has no cache locking, so a background prewarm downloading a pin can +# race a first-use pin_read() of the same pin and leave a truncated cache +# entry that poisons later reads. Both sides take an exclusive lock keyed by +# the board's cache path and pin name, making the cache single-writer: the +# reader waits out an in-flight download instead of duplicating it. +with_pin_lock <- function(board, pin, expr) { + cache <- board$cache + # Boards without a download cache (e.g. board_folder) never download, so + # there is no race to guard against. + if (is.null(cache) || is.na(cache) || !nzchar(cache)) { + return(force(expr)) + } + name <- gsub("[^A-Za-z0-9._-]", "_", pin) + dir.create(cache, recursive = TRUE, showWarnings = FALSE) + lock <- filelock::lock(file.path(cache, paste0("commons-", name, ".lock"))) + on.exit(filelock::unlock(lock), add = TRUE) + force(expr) +} + # Warm the pins on-disk cache in a background process rather than loading into # DuckDB: dbWriteTable() must run in this process (where the DuckDB lives) and # would block every question asked during the load. The board is serialized to @@ -531,7 +550,7 @@ prewarm_downloads <- function(board, pins) { function(pin) { tryCatch( { - pins::pin_download(board, pin) + with_pin_lock(board, pin, pins::pin_download(board, pin)) TRUE }, error = function(err) FALSE diff --git a/pkg-r/man/commons_prewarm.Rd b/pkg-r/man/commons_prewarm.Rd index 6d21d878..5cea3744 100644 --- a/pkg-r/man/commons_prewarm.Rd +++ b/pkg-r/man/commons_prewarm.Rd @@ -20,8 +20,12 @@ question: \details{ \itemize{ \item \code{agent$prewarm_context()} builds the context index (the store behind -\code{search_context}). It is synchronous and in-process: the index is -in-memory, so each Shiny session's agent builds its own. +\code{search_context}). The index is a persistent, content-addressed file, +so the build happens once per content version: later sessions open it +in milliseconds, and it can be built offline ahead of deployment. The +cache root resolves from the \code{commons.context_cache} option, the +\code{COMMONS_CONTEXT_CACHE} or \code{CONNECT_CONTENT_DATA_DIR} environment +variables, or the per-user cache directory, in that order. \item \code{agent$prewarm_sources()} starts a background process that downloads any uncached pins into the local pins cache (see \code{\link[=data_source]{data_source()}}). Because the pins cache is on disk, this can also run ahead of diff --git a/pkg-r/tests/testthat/setup-context-cache.R b/pkg-r/tests/testthat/setup-context-cache.R new file mode 100644 index 00000000..63922b97 --- /dev/null +++ b/pkg-r/tests/testthat/setup-context-cache.R @@ -0,0 +1,7 @@ +# Isolate the persistent context store cache per test session so stores +# built by one test session don't mask build spans (or leak) in another. +options(commons.context_cache = tempfile("commons-context-cache-")) +withr::defer( + unlink(getOption("commons.context_cache"), recursive = TRUE), + testthat::teardown_env() +) diff --git a/pkg-r/tests/testthat/test-commons.R b/pkg-r/tests/testthat/test-commons.R index d692a3b6..ec371cfe 100644 --- a/pkg-r/tests/testthat/test-commons.R +++ b/pkg-r/tests/testthat/test-commons.R @@ -464,6 +464,8 @@ test_that("prewarm() propagates failures", { test_that("prewarm_context() records a cache-miss build and its own span", { skip_if_not_installed("otelsdk") + # A fresh cache root guarantees a cold build regardless of test order. + withr::local_options(commons.context_cache = withr::local_tempdir()) path <- withr::local_tempfile(fileext = ".md") writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) @@ -483,6 +485,7 @@ test_that("prewarm_context() records a cache-miss build and its own span", { test_that("prewarm_context() records a cache hit without a build span", { skip_if_not_installed("otelsdk") + withr::local_options(commons.context_cache = withr::local_tempdir()) path <- withr::local_tempfile(fileext = ".md") writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) diff --git a/pkg-r/tests/testthat/test-context-layer.R b/pkg-r/tests/testthat/test-context-layer.R index d4f2019b..c7a1f2cd 100644 --- a/pkg-r/tests/testthat/test-context-layer.R +++ b/pkg-r/tests/testthat/test-context-layer.R @@ -77,3 +77,24 @@ test_that("context_layer skips a frontmatter-only file", { layer <- context_layer(files = path) expect_length(context_search(layer, "provenance"), 0) }) + +test_that("the context store persists on disk and is shared across layers", { + withr::local_options(commons.context_cache = withr::local_tempdir()) + path <- withr::local_tempfile(fileext = ".md") + writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) + + layer1 <- context_layer(files = path) + store_path <- context_store_path(layer1$docs) + expect_false(file.exists(store_path)) + + expect_match(context_search(layer1, "revenue")[[1]], "booked") + expect_true(file.exists(store_path)) + + # A distinct layer with the same docs opens the same on-disk store + layer2 <- context_layer(files = path) + expect_identical(context_store_path(layer2$docs), store_path) + expect_match(context_search(layer2, "revenue")[[1]], "booked") + + # Different docs key a different store + expect_false(identical(context_store_path("other docs"), store_path)) +}) From 8ed7c0476a9e3408caa322ed4d84a188f6af87a2 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 28 Aug 2026 14:39:59 -0500 Subject: [PATCH 04/11] Harden the context cache: app_cache resolution, pruning, fallbacks Adopting the caching lessons from sass/bslib/shiny/cachem: - Cache root resolution is now context-aware (sass's convention): a hosted Shiny app uses app_cache/commons beside the app, scoping the cache per application on shared hosts; a local app uses it only if it already exists. - Stores unused for commons.context_cache_max_age seconds (default 30 days) are pruned, throttled cachem-style (once per 20 builds or 5s). Opens touch the mtime so age approximates LRU. Content-addressed immutable files make eviction safe: an evicted store still works for sessions holding it open, and the next opener rebuilds. - An unwritable cache dir warns once and falls back to a per-session tempdir (sass's graceful degradation) -- caching never breaks the app. - options(commons.context_cache = FALSE) disables persistence for dev loops, building the index in memory per layer. cachem itself was considered and rejected as a backend: cache_disk stores RDS values with no path API, and its any-process-can-evict semantics conflict with shared read-only opens of a DuckDB file. --- pkg-r/R/chat.R | 6 +- pkg-r/R/commons.R | 3 +- pkg-r/R/context-layer.R | 134 ++++++++++++++++++++-- pkg-r/man/commons_prewarm.Rd | 6 +- pkg-r/tests/testthat/test-context-layer.R | 90 +++++++++++++++ 5 files changed, 229 insertions(+), 10 deletions(-) diff --git a/pkg-r/R/chat.R b/pkg-r/R/chat.R index a98f4bd7..7f933de1 100644 --- a/pkg-r/R/chat.R +++ b/pkg-r/R/chat.R @@ -135,7 +135,11 @@ commons_server <- function(id, client, ...) { #' in milliseconds, and it can be built offline ahead of deployment. The #' cache root resolves from the `commons.context_cache` option, the #' `COMMONS_CONTEXT_CACHE` or `CONNECT_CONTENT_DATA_DIR` environment -#' variables, or the per-user cache directory, in that order. +#' variables, an `app_cache/commons` directory beside a Shiny app, or the +#' per-user cache directory, in that order. Stores unused for 30 days are +#' pruned (option `commons.context_cache_max_age`, in seconds); set +#' `options(commons.context_cache = FALSE)` to disable persistence +#' entirely. #' * `agent$prewarm_sources()` starts a background process that downloads #' any uncached pins into the local pins cache (see [data_source()]). #' Because the pins cache is on disk, this can also run ahead of diff --git a/pkg-r/R/commons.R b/pkg-r/R/commons.R index fbf26dfc..943f7583 100644 --- a/pkg-r/R/commons.R +++ b/pkg-r/R/commons.R @@ -411,7 +411,8 @@ Commons <- R6::R6Class( "commons.context.n_docs" = length(layer_state$docs), "commons.context.cache_hit" = !is.null(layer_state$store) || - file.exists(context_store_path(layer_state$docs)) + (context_cache_enabled() && + file.exists(context_store_path(layer_state$docs))) ) ) context_store(layer) diff --git a/pkg-r/R/context-layer.R b/pkg-r/R/context-layer.R index e1470fe0..3eb5ba9a 100644 --- a/pkg-r/R/context-layer.R +++ b/pkg-r/R/context-layer.R @@ -114,20 +114,56 @@ strip_frontmatter <- function(md) { # conversations never search, but on a warm cache that first search only # pays for opening a file. Aliases of one layer share its store; augmenting # its documents creates a layer with a fresh store. +# Housekeeping state for the persistent context cache: prune throttling, the +# fallback tempdir, and the one-time warning about an unusable cache dir. +context_cache_state <- new.env(parent = emptyenv()) + context_store <- function(layer) { state <- context_layer_state(layer) if (!is.null(state$store)) { return(state$store) } + if (!context_cache_enabled()) { + store <- build_context_store_memory(state$docs) + state$store <- store + return(store) + } path <- context_store_path(state$docs) if (!file.exists(path)) { build_context_store(state$docs, path) + } else { + # Touch the mtime so age-based pruning approximates LRU: stores in active + # use stay young. Best-effort -- a read-only cache dir still opens fine. + tryCatch(Sys.setFileTime(path, Sys.time()), error = function(err) NULL) } store <- ragnar::ragnar_store_connect(path) state$store <- store store } +# options(commons.context_cache = FALSE) disables the persistent store (the +# index is built in memory per layer instead) -- an escape hatch for +# development loops over context files. +context_cache_enabled <- function() { + !identical(getOption("commons.context_cache"), FALSE) +} + +build_context_store_memory <- function(docs) { + local_commons_span( + "commons_context_store_build", + attributes = list( + "commons.context.n_docs" = length(docs), + "commons.context.persistent" = FALSE + ) + ) + store <- ragnar::ragnar_store_create(embed = NULL) + for (doc in docs) { + ragnar::ragnar_store_insert(store, ragnar::markdown_chunk(doc)) + } + ragnar::ragnar_store_build_index(store, type = "fts") + store +} + # Build to a temp file in the same directory, then rename into place # atomically, so a concurrent reader or builder never observes a partial # store. If another builder wins the race, its store is equivalent content; @@ -135,7 +171,10 @@ context_store <- function(layer) { build_context_store <- function(docs, path) { local_commons_span( "commons_context_store_build", - attributes = list("commons.context.n_docs" = length(docs)) + attributes = list( + "commons.context.n_docs" = length(docs), + "commons.context.persistent" = TRUE + ) ) dir.create(dirname(path), recursive = TRUE, showWarnings = FALSE) tmp <- tempfile(pattern = ".build-", tmpdir = dirname(path)) @@ -154,26 +193,56 @@ build_context_store <- function(docs, path) { if (!file.exists(path)) { cli::cli_abort("Failed to build the context store at {.path {path}}.") } + prune_context_cache(dirname(path)) invisible(path) } +# Content-addressed stores accumulate one file per content version, so prune +# by age. The mtime touch on open makes age approximate LRU. Throttled like +# cachem (at most once per 20 builds or per 5 seconds) since stat-ing the +# directory on every build is needlessly slow; concurrent pruners may +# double-delete, which unlink tolerates with a warning. +prune_context_cache <- function( + dir, + max_age = getOption("commons.context_cache_max_age", 30 * 24 * 60 * 60) +) { + now <- Sys.time() + context_cache_state$n_builds <- (context_cache_state$n_builds %||% 0) + 1 + last <- context_cache_state$last_prune + throttled <- context_cache_state$n_builds %% 20 != 0 && + !is.null(last) && + difftime(now, last, units = "secs") < 5 + if (throttled) { + return(invisible()) + } + context_cache_state$last_prune <- now + + stores <- list.files(dir, pattern = "[.]duckdb$", full.names = TRUE) + old <- stores[file.mtime(stores) < now - max_age] + suppressWarnings(unlink(old)) + invisible() +} + context_store_path <- function(docs) { key <- rlang::hash(c( docs, paste0("ragnar:", utils::packageVersion("ragnar")), paste0("duckdb:", utils::packageVersion("duckdb")) )) - file.path(context_cache_dir(), "context", paste0(key, ".duckdb")) + file.path(context_cache_dir_safe(), "context", paste0(key, ".duckdb")) } # Cache root resolution: an explicit override, then Connect's persistent -# data directory (survives deployments when the server enables it), then the -# per-user cache dir. Wherever the root is ephemeral (e.g. Connect Cloud, -# which resets disk to the deployed bundle), the store simply rebuilds once -# per cache lifetime instead of once per process. +# data directory (survives deployments when the server enables it), then -- +# for Shiny apps -- an app_cache/ directory beside the app (sass's +# convention: per-app scoping on hosted platforms, used locally only if it +# already exists), then the per-user cache dir. Wherever the root is +# ephemeral (e.g. Connect Cloud, which resets disk to the deployed bundle), +# the store simply rebuilds once per cache lifetime instead of once per +# process. context_cache_dir <- function() { opt <- getOption("commons.context_cache") - if (!is.null(opt)) { + if (!is.null(opt) && !identical(opt, FALSE)) { return(opt) } for (env in c("COMMONS_CONTEXT_CACHE", "CONNECT_CONTENT_DATA_DIR")) { @@ -182,9 +251,60 @@ context_cache_dir <- function() { return(val) } } + if (is_shiny_app()) { + app_dir <- shiny::getShinyOption("appDir") + if (!is.null(app_dir)) { + app_cache <- file.path(app_dir, "app_cache", "commons") + if ( + is_hosted_shiny_app() || + dir.exists(app_cache) || + dir.exists(dirname(app_cache)) + ) { + return(app_cache) + } + } + } tools::R_user_dir("commons", "cache") } +is_shiny_app <- function() { + isNamespaceLoaded("shiny") && shiny::isRunning() +} + +# Connect and Shiny Server both set SHINY_SERVER_VERSION for content. +is_hosted_shiny_app <- function() { + nzchar(Sys.getenv("SHINY_SERVER_VERSION")) && is_shiny_app() +} + +# Caching must never take down the app: if the resolved cache dir can't be +# created or written, warn once and fall back to a per-session tempdir (the +# store becomes per-process, as if persistence were disabled). +context_cache_dir_safe <- function() { + dir <- context_cache_dir() + ok <- tryCatch( + { + dir.create(dir, recursive = TRUE, showWarnings = FALSE) + dir.exists(dir) && file.access(dir, 2) == 0 + }, + error = function(err) FALSE + ) + if (ok) { + return(dir) + } + if (is.null(context_cache_state$warned)) { + context_cache_state$warned <- TRUE + cli::cli_warn(c( + "Cannot write to the context cache directory {.path {dir}}.", + i = "Falling back to a per-session temporary directory; the context index will be rebuilt in each process." + )) + } + if (is.null(context_cache_state$fallback_dir)) { + context_cache_state$fallback_dir <- tempfile("commons-context-cache-") + dir.create(context_cache_state$fallback_dir, recursive = TRUE) + } + context_cache_state$fallback_dir +} + context_search <- function(layer, query, n = 3) { state <- context_layer_state(layer) if (length(state$docs) == 0) { diff --git a/pkg-r/man/commons_prewarm.Rd b/pkg-r/man/commons_prewarm.Rd index 5cea3744..10bb53a5 100644 --- a/pkg-r/man/commons_prewarm.Rd +++ b/pkg-r/man/commons_prewarm.Rd @@ -25,7 +25,11 @@ so the build happens once per content version: later sessions open it in milliseconds, and it can be built offline ahead of deployment. The cache root resolves from the \code{commons.context_cache} option, the \code{COMMONS_CONTEXT_CACHE} or \code{CONNECT_CONTENT_DATA_DIR} environment -variables, or the per-user cache directory, in that order. +variables, an \code{app_cache/commons} directory beside a Shiny app, or the +per-user cache directory, in that order. Stores unused for 30 days are +pruned (option \code{commons.context_cache_max_age}, in seconds); set +\code{options(commons.context_cache = FALSE)} to disable persistence +entirely. \item \code{agent$prewarm_sources()} starts a background process that downloads any uncached pins into the local pins cache (see \code{\link[=data_source]{data_source()}}). Because the pins cache is on disk, this can also run ahead of diff --git a/pkg-r/tests/testthat/test-context-layer.R b/pkg-r/tests/testthat/test-context-layer.R index c7a1f2cd..3fa6bfbd 100644 --- a/pkg-r/tests/testthat/test-context-layer.R +++ b/pkg-r/tests/testthat/test-context-layer.R @@ -98,3 +98,93 @@ test_that("the context store persists on disk and is shared across layers", { # Different docs key a different store expect_false(identical(context_store_path("other docs"), store_path)) }) + +test_that("commons.context_cache = FALSE builds the store in memory", { + withr::local_options(commons.context_cache = FALSE) + path <- withr::local_tempfile(fileext = ".md") + writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) + + layer <- context_layer(files = path) + expect_match(context_search(layer, "revenue")[[1]], "booked") + expect_identical( + DBI::dbGetInfo(context_layer_state(layer)$store@con)$dbname, + ":memory:" + ) +}) + +test_that("an unwritable cache dir warns once and falls back to a tempdir", { + # A file where the cache dir should be makes dir.create() fail. + blocker <- withr::local_tempfile() + writeLines("occupied", blocker) + withr::local_options(commons.context_cache = file.path(blocker, "cache")) + + context_cache_state$warned <- NULL + context_cache_state$fallback_dir <- NULL + + path <- withr::local_tempfile(fileext = ".md") + writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) + layer <- context_layer(files = path) + + expect_warning( + expect_match(context_search(layer, "revenue")[[1]], "booked"), + "Falling back to a per-session temporary directory" + ) + # The fallback is stable within the session and warns only once + expect_no_warning(context_cache_dir_safe()) + expect_identical(context_cache_dir_safe(), context_cache_state$fallback_dir) +}) + +test_that("prune_context_cache() removes old stores and keeps young ones", { + dir <- withr::local_tempdir() + old <- file.path(dir, "old.duckdb") + young <- file.path(dir, "young.duckdb") + file.create(old, young) + Sys.setFileTime(old, Sys.time() - 40 * 24 * 60 * 60) + + context_cache_state$n_builds <- 0 + context_cache_state$last_prune <- NULL + prune_context_cache(dir) + + expect_false(file.exists(old)) + expect_true(file.exists(young)) +}) + +test_that("prune_context_cache() is throttled across builds", { + dir <- withr::local_tempdir() + stale <- file.path(dir, "stale.duckdb") + file.create(stale) + Sys.setFileTime(stale, Sys.time() - 40 * 24 * 60 * 60) + + # A prune just happened, and the build count isn't at a multiple of 20 + context_cache_state$n_builds <- 1 + context_cache_state$last_prune <- Sys.time() + prune_context_cache(dir) + expect_true(file.exists(stale)) + + # Twenty builds since the throttle reset forces a prune + context_cache_state$n_builds <- 19 + prune_context_cache(dir) + expect_false(file.exists(stale)) +}) + +test_that("cache root prefers the option, then env vars", { + withr::local_options(commons.context_cache = NULL) + withr::local_envvar( + COMMONS_CONTEXT_CACHE = NA, + CONNECT_CONTENT_DATA_DIR = NA, + SHINY_SERVER_VERSION = NA + ) + expect_identical( + context_cache_dir(), + tools::R_user_dir("commons", "cache") + ) + + withr::local_envvar(CONNECT_CONTENT_DATA_DIR = "/connect/data") + expect_identical(context_cache_dir(), "/connect/data") + + withr::local_envvar(COMMONS_CONTEXT_CACHE = "/explicit/cache") + expect_identical(context_cache_dir(), "/explicit/cache") + + withr::local_options(commons.context_cache = "/option/cache") + expect_identical(context_cache_dir(), "/option/cache") +}) From 0365e689802092fd76a78f157c3077b59fb53ef0 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 28 Aug 2026 14:42:41 -0500 Subject: [PATCH 05/11] Document app_cache bundle exclusion and the ship-a-warm-store path rsconnect unconditionally excludes app_cache/ from deployed bundles (bundleFiles.R ignoreBundleFiles), so the app_cache cache root is per-deployment, not cross-deployment. Shipping a pre-built store with the app means pointing commons.context_cache at a bundle-included directory and running prewarm_context() before deploy. --- pkg-r/R/chat.R | 7 ++++++- pkg-r/man/commons_prewarm.Rd | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/pkg-r/R/chat.R b/pkg-r/R/chat.R index 7f933de1..8253ea34 100644 --- a/pkg-r/R/chat.R +++ b/pkg-r/R/chat.R @@ -136,7 +136,12 @@ commons_server <- function(id, client, ...) { #' cache root resolves from the `commons.context_cache` option, the #' `COMMONS_CONTEXT_CACHE` or `CONNECT_CONTENT_DATA_DIR` environment #' variables, an `app_cache/commons` directory beside a Shiny app, or the -#' per-user cache directory, in that order. Stores unused for 30 days are +#' per-user cache directory, in that order. Note that `app_cache/` is +#' excluded from deployed bundles (rsconnect treats it as server-side +#' scratch), so it is shared across the sessions of one deployment but +#' rebuilt after a redeploy; to ship a pre-built store with the app, +#' point `commons.context_cache` at a directory inside the app and run +#' `prewarm_context()` before deploying. Stores unused for 30 days are #' pruned (option `commons.context_cache_max_age`, in seconds); set #' `options(commons.context_cache = FALSE)` to disable persistence #' entirely. diff --git a/pkg-r/man/commons_prewarm.Rd b/pkg-r/man/commons_prewarm.Rd index 10bb53a5..dacd9826 100644 --- a/pkg-r/man/commons_prewarm.Rd +++ b/pkg-r/man/commons_prewarm.Rd @@ -26,7 +26,12 @@ in milliseconds, and it can be built offline ahead of deployment. The cache root resolves from the \code{commons.context_cache} option, the \code{COMMONS_CONTEXT_CACHE} or \code{CONNECT_CONTENT_DATA_DIR} environment variables, an \code{app_cache/commons} directory beside a Shiny app, or the -per-user cache directory, in that order. Stores unused for 30 days are +per-user cache directory, in that order. Note that \verb{app_cache/} is +excluded from deployed bundles (rsconnect treats it as server-side +scratch), so it is shared across the sessions of one deployment but +rebuilt after a redeploy; to ship a pre-built store with the app, +point \code{commons.context_cache} at a directory inside the app and run +\code{prewarm_context()} before deploying. Stores unused for 30 days are pruned (option \code{commons.context_cache_max_age}, in seconds); set \code{options(commons.context_cache = FALSE)} to disable persistence entirely. From 5f387c62e5413db147cf2d5aa5cd9bdf17292751 Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 31 Aug 2026 17:22:26 -0500 Subject: [PATCH 06/11] Harden context cache configuration, pruning, and prewarm entry points Review-driven fixes on top of the persistent context store: - Validate options(commons.context_cache): a non-string, non-FALSE value now aborts instead of creating a directory named after the value. - Treat COMMONS_CONTEXT_CACHE=false/0/no (any case) as disabling the cache; env vars can't express FALSE, and a literal "FALSE" directory was previously created. - commons_prewarm() warms synchronously when no Shiny event loop is running (e.g. pre-deploy scripts), where a later::later() callback would never fire. - Wrap the prewarm span's cache_hit attribute in tryCatch so telemetry can never abort prewarming. - Replace age-based pruning with a single size-cap knob: options(commons.context_cache_max_size) (default 256 MB) with LRU eviction by mtime (touched on open). The just-built store is explicitly protected, and a single store larger than the cap is kept with a one-time warning, matching cachem's behavior, rather than evicted into a rebuild loop. - Fix the persistent-store test to read docs via context_layer_state(); layer$docs returns NULL now that layer internals are private. - Credit the sass package's file cache as prior art in the commons_prewarm() docs. --- pkg-r/R/chat.R | 31 +++++-- pkg-r/R/commons.R | 9 +- pkg-r/R/context-layer.R | 72 +++++++++++++--- pkg-r/man/commons_prewarm.Rd | 20 +++-- pkg-r/tests/testthat/_snaps/layer-objects.md | 1 + pkg-r/tests/testthat/test-chat.R | 4 +- pkg-r/tests/testthat/test-context-layer.R | 91 +++++++++++++++----- 7 files changed, 176 insertions(+), 52 deletions(-) diff --git a/pkg-r/R/chat.R b/pkg-r/R/chat.R index 8253ea34..3c0f5c66 100644 --- a/pkg-r/R/chat.R +++ b/pkg-r/R/chat.R @@ -136,15 +136,21 @@ commons_server <- function(id, client, ...) { #' cache root resolves from the `commons.context_cache` option, the #' `COMMONS_CONTEXT_CACHE` or `CONNECT_CONTENT_DATA_DIR` environment #' variables, an `app_cache/commons` directory beside a Shiny app, or the -#' per-user cache directory, in that order. Note that `app_cache/` is +#' per-user cache directory, in that order. This resolution ladder (and +#' the content-addressed, age-pruned cache design generally) follows the +#' file cache in the sass package, which has run in production Shiny +#' deployments for years. Note that `app_cache/` is #' excluded from deployed bundles (rsconnect treats it as server-side #' scratch), so it is shared across the sessions of one deployment but #' rebuilt after a redeploy; to ship a pre-built store with the app, #' point `commons.context_cache` at a directory inside the app and run -#' `prewarm_context()` before deploying. Stores unused for 30 days are -#' pruned (option `commons.context_cache_max_age`, in seconds); set -#' `options(commons.context_cache = FALSE)` to disable persistence -#' entirely. +#' `prewarm_context()` before deploying. The cache is capped at 256 MB +#' with least-recently-used eviction (option +#' `commons.context_cache_max_size`, in bytes; a single store larger than +#' the cap is kept, with a warning). Set +#' `options(commons.context_cache = FALSE)` (or the +#' `COMMONS_CONTEXT_CACHE` environment variable to `false`) to disable +#' persistence entirely. #' * `agent$prewarm_sources()` starts a background process that downloads #' any uncached pins into the local pins cache (see [data_source()]). #' Because the pins cache is on disk, this can also run ahead of @@ -153,7 +159,9 @@ commons_server <- function(id, client, ...) { #' #' `agent$prewarm()` calls both. Call `commons_prewarm()` in a Shiny server #' function to defer warming to post-startup idle time, so it happens while -#' the user reads the welcome message. +#' the user reads the welcome message. Outside a running Shiny app (e.g. a +#' pre-deploy warm-up script) there is no [later::later()] event loop, so +#' `commons_prewarm()` warms synchronously instead. #' #' `prewarm()` lets failures propagate, since a direct call is typically #' warming caches ahead of deployment and a mere warning would sail through @@ -183,12 +191,19 @@ commons_prewarm <- function(client) { check_commons_client(client) # An error escaping a later::later() callback stops the Shiny app, and # pre-warming is a pure optimization, so downgrade failures to warnings. - later::later(function() { + warm <- function() { tryCatch( client$prewarm(), error = function(err) cli::cli_warn(conditionMessage(err)) ) - }) + } + # later::later() only fires while an event loop is running; outside Shiny + # (e.g. a pre-deploy warm-up script) the callback would never run. + if (is_shiny_app()) { + later::later(warm) + } else { + warm() + } invisible(NULL) } diff --git a/pkg-r/R/commons.R b/pkg-r/R/commons.R index 943f7583..a772f689 100644 --- a/pkg-r/R/commons.R +++ b/pkg-r/R/commons.R @@ -409,10 +409,15 @@ Commons <- R6::R6Class( "commons_context_prewarm", attributes = list( "commons.context.n_docs" = length(layer_state$docs), + # tryCatch: telemetry must not abort prewarming (resolving the + # cache dir can fail or warn on an unwritable root). "commons.context.cache_hit" = !is.null(layer_state$store) || - (context_cache_enabled() && - file.exists(context_store_path(layer_state$docs))) + isTRUE(tryCatch( + context_cache_enabled() && + file.exists(context_store_path(layer_state$docs)), + error = function(err) FALSE + )) ) ) context_store(layer) diff --git a/pkg-r/R/context-layer.R b/pkg-r/R/context-layer.R index 3eb5ba9a..54634f2c 100644 --- a/pkg-r/R/context-layer.R +++ b/pkg-r/R/context-layer.R @@ -132,8 +132,8 @@ context_store <- function(layer) { if (!file.exists(path)) { build_context_store(state$docs, path) } else { - # Touch the mtime so age-based pruning approximates LRU: stores in active - # use stay young. Best-effort -- a read-only cache dir still opens fine. + # Touch the mtime so size-cap eviction is LRU: stores in active use + # stay young. Best-effort -- a read-only cache dir still opens fine. tryCatch(Sys.setFileTime(path, Sys.time()), error = function(err) NULL) } store <- ragnar::ragnar_store_connect(path) @@ -145,7 +145,15 @@ context_store <- function(layer) { # index is built in memory per layer instead) -- an escape hatch for # development loops over context files. context_cache_enabled <- function() { - !identical(getOption("commons.context_cache"), FALSE) + if (identical(getOption("commons.context_cache"), FALSE)) { + return(FALSE) + } + # Env vars can't express FALSE; accept the usual spellings. + val <- Sys.getenv("COMMONS_CONTEXT_CACHE", unset = NA_character_) + if (!is.na(val) && tolower(val) %in% c("false", "0", "no")) { + return(FALSE) + } + TRUE } build_context_store_memory <- function(docs) { @@ -193,18 +201,24 @@ build_context_store <- function(docs, path) { if (!file.exists(path)) { cli::cli_abort("Failed to build the context store at {.path {path}}.") } - prune_context_cache(dirname(path)) + prune_context_cache(dirname(path), protect = path) invisible(path) } -# Content-addressed stores accumulate one file per content version, so prune -# by age. The mtime touch on open makes age approximate LRU. Throttled like -# cachem (at most once per 20 builds or per 5 seconds) since stat-ing the -# directory on every build is needlessly slow; concurrent pruners may -# double-delete, which unlink tolerates with a warning. +# Content-addressed stores accumulate one file per content version, so the +# cache is capped by total size and pruned LRU: the mtime touch on open +# keeps actively used stores young, so eviction deletes least-recently-used +# stores first. The store just built is protected explicitly (not just by +# its young mtime) so a store larger than the cap survives: like cachem, a +# single oversized store is kept, with a one-time warning, rather than +# evicted into a rebuild loop. Throttled like cachem (at most once per 20 +# builds or per 5 seconds) since stat-ing the directory on every build is +# needlessly slow; concurrent pruners may double-delete, which unlink +# tolerates with a warning. prune_context_cache <- function( dir, - max_age = getOption("commons.context_cache_max_age", 30 * 24 * 60 * 60) + max_size = getOption("commons.context_cache_max_size", 256 * 1024^2), + protect = NULL ) { now <- Sys.time() context_cache_state$n_builds <- (context_cache_state$n_builds %||% 0) + 1 @@ -217,12 +231,38 @@ prune_context_cache <- function( } context_cache_state$last_prune <- now + # Evict least-recently-used stores until the cache fits under max_size. stores <- list.files(dir, pattern = "[.]duckdb$", full.names = TRUE) - old <- stores[file.mtime(stores) < now - max_age] - suppressWarnings(unlink(old)) + total <- sum(file.size(stores)) + if (total > max_size) { + evictable <- setdiff(stores, protect) + evictable <- evictable[order(file.mtime(evictable))] + for (victim in evictable) { + if (total <= max_size) { + break + } + total <- total - file.size(victim) + suppressWarnings(unlink(victim)) + } + if (total > max_size && is.null(context_cache_state$warned_size)) { + context_cache_state$warned_size <- TRUE + cli::cli_warn(c( + "The context cache exceeds its size cap ({format_size(max_size)}) with only protected or in-use stores remaining.", + i = "A single store larger than the cap is kept; raise {.code options(commons.context_cache_max_size)} if this is expected." + )) + } + } invisible() } +format_size <- function(bytes) { + if (bytes >= 1024^2) { + sprintf("%.0f MB", bytes / 1024^2) + } else { + sprintf("%.0f KB", bytes / 1024) + } +} + context_store_path <- function(docs) { key <- rlang::hash(c( docs, @@ -243,11 +283,17 @@ context_store_path <- function(docs) { context_cache_dir <- function() { opt <- getOption("commons.context_cache") if (!is.null(opt) && !identical(opt, FALSE)) { + if (!rlang::is_string(opt)) { + cli::cli_abort( + "{.code options(commons.context_cache)} must be a path to a cache directory or {.code FALSE}." + ) + } return(opt) } for (env in c("COMMONS_CONTEXT_CACHE", "CONNECT_CONTENT_DATA_DIR")) { val <- Sys.getenv(env, unset = NA_character_) - if (!is.na(val) && nzchar(val)) { + false_like <- !is.na(val) && tolower(val) %in% c("false", "0", "no") + if (!is.na(val) && nzchar(val) && !false_like) { return(val) } } diff --git a/pkg-r/man/commons_prewarm.Rd b/pkg-r/man/commons_prewarm.Rd index dacd9826..92e5b30a 100644 --- a/pkg-r/man/commons_prewarm.Rd +++ b/pkg-r/man/commons_prewarm.Rd @@ -26,15 +26,21 @@ in milliseconds, and it can be built offline ahead of deployment. The cache root resolves from the \code{commons.context_cache} option, the \code{COMMONS_CONTEXT_CACHE} or \code{CONNECT_CONTENT_DATA_DIR} environment variables, an \code{app_cache/commons} directory beside a Shiny app, or the -per-user cache directory, in that order. Note that \verb{app_cache/} is +per-user cache directory, in that order. This resolution ladder (and +the content-addressed, age-pruned cache design generally) follows the +file cache in the sass package, which has run in production Shiny +deployments for years. Note that \verb{app_cache/} is excluded from deployed bundles (rsconnect treats it as server-side scratch), so it is shared across the sessions of one deployment but rebuilt after a redeploy; to ship a pre-built store with the app, point \code{commons.context_cache} at a directory inside the app and run -\code{prewarm_context()} before deploying. Stores unused for 30 days are -pruned (option \code{commons.context_cache_max_age}, in seconds); set -\code{options(commons.context_cache = FALSE)} to disable persistence -entirely. +\code{prewarm_context()} before deploying. The cache is capped at 256 MB +with least-recently-used eviction (option +\code{commons.context_cache_max_size}, in bytes; a single store larger than +the cap is kept, with a warning). Set +\code{options(commons.context_cache = FALSE)} (or the +\code{COMMONS_CONTEXT_CACHE} environment variable to \code{false}) to disable +persistence entirely. \item \code{agent$prewarm_sources()} starts a background process that downloads any uncached pins into the local pins cache (see \code{\link[=data_source]{data_source()}}). Because the pins cache is on disk, this can also run ahead of @@ -44,7 +50,9 @@ app reads the warmed cache. \code{agent$prewarm()} calls both. Call \code{commons_prewarm()} in a Shiny server function to defer warming to post-startup idle time, so it happens while -the user reads the welcome message. +the user reads the welcome message. Outside a running Shiny app (e.g. a +pre-deploy warm-up script) there is no \code{\link[later:later]{later::later()}} event loop, so +\code{commons_prewarm()} warms synchronously instead. \code{prewarm()} lets failures propagate, since a direct call is typically warming caches ahead of deployment and a mere warning would sail through diff --git a/pkg-r/tests/testthat/_snaps/layer-objects.md b/pkg-r/tests/testthat/_snaps/layer-objects.md index a3755fc8..af340f03 100644 --- a/pkg-r/tests/testthat/_snaps/layer-objects.md +++ b/pkg-r/tests/testthat/_snaps/layer-objects.md @@ -36,3 +36,4 @@ print(context_two) Message A commons context layer with 2 documents. + diff --git a/pkg-r/tests/testthat/test-chat.R b/pkg-r/tests/testthat/test-chat.R index dc0845d4..89aade80 100644 --- a/pkg-r/tests/testthat/test-chat.R +++ b/pkg-r/tests/testthat/test-chat.R @@ -115,8 +115,8 @@ test_that("commons_prewarm() downgrades prewarm failures to warnings", { context_store = function(...) stop("index build exploded"), .package = "commons" ) - commons_prewarm(agent) - expect_warning(later::run_now(), "index build exploded") + # Outside a running Shiny app, commons_prewarm() warms synchronously. + expect_warning(commons_prewarm(agent), "index build exploded") }) test_that("commons_app() prewarms the agent on idle", { diff --git a/pkg-r/tests/testthat/test-context-layer.R b/pkg-r/tests/testthat/test-context-layer.R index 3fa6bfbd..5d96d372 100644 --- a/pkg-r/tests/testthat/test-context-layer.R +++ b/pkg-r/tests/testthat/test-context-layer.R @@ -84,7 +84,7 @@ test_that("the context store persists on disk and is shared across layers", { writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) layer1 <- context_layer(files = path) - store_path <- context_store_path(layer1$docs) + store_path <- context_store_path(context_layer_state(layer1)$docs) expect_false(file.exists(store_path)) expect_match(context_search(layer1, "revenue")[[1]], "booked") @@ -92,7 +92,10 @@ test_that("the context store persists on disk and is shared across layers", { # A distinct layer with the same docs opens the same on-disk store layer2 <- context_layer(files = path) - expect_identical(context_store_path(layer2$docs), store_path) + expect_identical( + context_store_path(context_layer_state(layer2)$docs), + store_path + ) expect_match(context_search(layer2, "revenue")[[1]], "booked") # Different docs key a different store @@ -134,39 +137,71 @@ test_that("an unwritable cache dir warns once and falls back to a tempdir", { expect_identical(context_cache_dir_safe(), context_cache_state$fallback_dir) }) -test_that("prune_context_cache() removes old stores and keeps young ones", { - dir <- withr::local_tempdir() - old <- file.path(dir, "old.duckdb") - young <- file.path(dir, "young.duckdb") - file.create(old, young) - Sys.setFileTime(old, Sys.time() - 40 * 24 * 60 * 60) - - context_cache_state$n_builds <- 0 - context_cache_state$last_prune <- NULL - prune_context_cache(dir) - - expect_false(file.exists(old)) - expect_true(file.exists(young)) -}) - test_that("prune_context_cache() is throttled across builds", { dir <- withr::local_tempdir() stale <- file.path(dir, "stale.duckdb") - file.create(stale) - Sys.setFileTime(stale, Sys.time() - 40 * 24 * 60 * 60) + writeLines(strrep("x", 1000), stale) # A prune just happened, and the build count isn't at a multiple of 20 context_cache_state$n_builds <- 1 context_cache_state$last_prune <- Sys.time() - prune_context_cache(dir) + prune_context_cache(dir, max_size = 1) expect_true(file.exists(stale)) # Twenty builds since the throttle reset forces a prune context_cache_state$n_builds <- 19 - prune_context_cache(dir) + prune_context_cache(dir, max_size = 1) expect_false(file.exists(stale)) }) +test_that("prune_context_cache() evicts least-recently-used stores over the size cap", { + dir <- withr::local_tempdir() + oldest <- file.path(dir, "oldest.duckdb") + middle <- file.path(dir, "middle.duckdb") + newest <- file.path(dir, "newest.duckdb") + for (f in c(oldest, middle, newest)) { + writeLines(strrep("x", 1000), f) + } + now <- Sys.time() + Sys.setFileTime(oldest, now - 300) + Sys.setFileTime(middle, now - 200) + Sys.setFileTime(newest, now - 100) + + context_cache_state$n_builds <- 0 + context_cache_state$last_prune <- NULL + # Cap fits two stores; the oldest is evicted + cap <- 2 * file.size(newest) + prune_context_cache(dir, max_size = cap) + + expect_false(file.exists(oldest)) + expect_true(file.exists(middle)) + expect_true(file.exists(newest)) +}) + +test_that("prune_context_cache() keeps a single store larger than the cap", { + dir <- withr::local_tempdir() + big <- file.path(dir, "big.duckdb") + writeLines(strrep("x", 10000), big) + + context_cache_state$n_builds <- 0 + context_cache_state$last_prune <- NULL + context_cache_state$warned_size <- NULL + + expect_warning( + prune_context_cache(dir, max_size = 1, protect = big), + "exceeds its size cap" + ) + expect_true(file.exists(big)) + + # Warns only once per session + context_cache_state$n_builds <- 0 + context_cache_state$last_prune <- NULL + expect_no_warning( + prune_context_cache(dir, max_size = 1, protect = big) + ) + expect_true(file.exists(big)) +}) + test_that("cache root prefers the option, then env vars", { withr::local_options(commons.context_cache = NULL) withr::local_envvar( @@ -188,3 +223,17 @@ test_that("cache root prefers the option, then env vars", { withr::local_options(commons.context_cache = "/option/cache") expect_identical(context_cache_dir(), "/option/cache") }) + +test_that("the context_cache option must be a path or FALSE", { + withr::local_options(commons.context_cache = TRUE) + expect_error(context_cache_dir(), "must be a path") +}) + +test_that("COMMONS_CONTEXT_CACHE can disable the cache", { + withr::local_options(commons.context_cache = NULL) + withr::local_envvar(COMMONS_CONTEXT_CACHE = "FALSE") + expect_false(context_cache_enabled()) + # A false-like value is never mistaken for a path + withr::local_envvar(CONNECT_CONTENT_DATA_DIR = "/connect/data") + expect_identical(context_cache_dir(), "/connect/data") +}) From 38e16697a6a887ce392246ab0532fdabb6302f49 Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 31 Aug 2026 17:32:23 -0500 Subject: [PATCH 07/11] Harden context cache: reap stale build files, recover from unopenable stores - prune_context_cache() reaps .build-* temp files older than 24h so crashed builds can't leak partial stores outside the size cap, and only decrements the size total when an eviction unlink succeeds. - context_store() warns (and notifies in Shiny) and rebuilds once when the cached store fails to open, e.g. unlinked by a concurrent pruner; a second failure still propagates. - Note the pins-version assumption in with_pin_lock() and clarify in ?commons_prewarm that failures are downgraded even on the synchronous path. --- pkg-r/R/chat.R | 5 +- pkg-r/R/context-layer.R | 64 +++++++++++++++++++++-- pkg-r/R/data-source.R | 2 + pkg-r/man/commons_prewarm.Rd | 5 +- pkg-r/tests/testthat/test-context-layer.R | 38 ++++++++++++++ 5 files changed, 108 insertions(+), 6 deletions(-) diff --git a/pkg-r/R/chat.R b/pkg-r/R/chat.R index 3c0f5c66..dbd35a74 100644 --- a/pkg-r/R/chat.R +++ b/pkg-r/R/chat.R @@ -161,7 +161,10 @@ commons_server <- function(id, client, ...) { #' function to defer warming to post-startup idle time, so it happens while #' the user reads the welcome message. Outside a running Shiny app (e.g. a #' pre-deploy warm-up script) there is no [later::later()] event loop, so -#' `commons_prewarm()` warms synchronously instead. +#' `commons_prewarm()` warms synchronously instead. Note that +#' `commons_prewarm()` always downgrades failures to warnings (see below), +#' even on this synchronous path — a pre-deploy script that should fail the +#' deploy on a cold cache must call `agent$prewarm()` directly. #' #' `prewarm()` lets failures propagate, since a direct call is typically #' warming caches ahead of deployment and a mere warning would sail through diff --git a/pkg-r/R/context-layer.R b/pkg-r/R/context-layer.R index 54634f2c..f8570e87 100644 --- a/pkg-r/R/context-layer.R +++ b/pkg-r/R/context-layer.R @@ -136,11 +136,45 @@ context_store <- function(layer) { # stay young. Best-effort -- a read-only cache dir still opens fine. tryCatch(Sys.setFileTime(path, Sys.time()), error = function(err) NULL) } - store <- ragnar::ragnar_store_connect(path) + store <- tryCatch( + ragnar::ragnar_store_connect(path), + error = function(err) err + ) + if (inherits(store, "error")) { + # Typically a concurrent pruner unlinked the store between our + # file.exists() and the connect; a corrupt store file is also possible. + # Warn (and notify in Shiny, where a warning is easy to miss), then + # rebuild once. A second failure propagates, so persistent corruption + # still surfaces rather than being silently rebuilt every session. + context_store_connect_warning(path, store) + unlink(path) + build_context_store(state$docs, path) + store <- ragnar::ragnar_store_connect(path) + } state$store <- store store } +context_store_connect_warning <- function(path, err) { + # Assign first: the raw message can contain braces (DuckDB errors embed + # JSON), which cli would try to interpolate. + detail <- conditionMessage(err) + cli::cli_warn(c( + "Failed to open the cached context store at {.path {path}}; rebuilding it.", + i = "{detail}" + )) + if (is_shiny_app()) { + tryCatch( + shiny::showNotification( + "The context index is being rebuilt; the first search may be slow.", + type = "warning", + duration = 8 + ), + error = function(err) NULL + ) + } +} + # options(commons.context_cache = FALSE) disables the persistent store (the # index is built in memory per layer instead) -- an escape hatch for # development loops over context files. @@ -231,7 +265,8 @@ prune_context_cache <- function( } context_cache_state$last_prune <- now - # Evict least-recently-used stores until the cache fits under max_size. + reap_stale_build_files(dir, now) + stores <- list.files(dir, pattern = "[.]duckdb$", full.names = TRUE) total <- sum(file.size(stores)) if (total > max_size) { @@ -241,8 +276,12 @@ prune_context_cache <- function( if (total <= max_size) { break } - total <- total - file.size(victim) - suppressWarnings(unlink(victim)) + size <- file.size(victim) + # Only count the eviction when the unlink actually happened -- on + # Windows, deleting a store another process holds open fails. + if (suppressWarnings(unlink(victim)) == 0) { + total <- total - size + } } if (total > max_size && is.null(context_cache_state$warned_size)) { context_cache_state$warned_size <- TRUE @@ -255,6 +294,23 @@ prune_context_cache <- function( invisible() } +# build_context_store() builds at a `.build-*` temp file that the size-cap +# pruner never sees (it lists `*.duckdb`), so a crashed or killed build would +# otherwise leak its partial store forever. Reap temp files older than a day: +# young enough to clear debris promptly, old enough to never delete a build +# that is still in flight. +reap_stale_build_files <- function(dir, now, max_age = 24 * 60 * 60) { + stale <- list.files( + dir, + pattern = "^[.]build-", + all.files = TRUE, + full.names = TRUE + ) + old <- stale[difftime(now, file.mtime(stale), units = "secs") > max_age] + suppressWarnings(unlink(old, recursive = TRUE)) + invisible() +} + format_size <- function(bytes) { if (bytes >= 1024^2) { sprintf("%.0f MB", bytes / 1024^2) diff --git a/pkg-r/R/data-source.R b/pkg-r/R/data-source.R index 7402098f..66044516 100644 --- a/pkg-r/R/data-source.R +++ b/pkg-r/R/data-source.R @@ -493,6 +493,8 @@ source_ensure_all <- function(source, call = rlang::caller_env()) { # the board's cache path and pin name, making the cache single-writer: the # reader waits out an in-flight download instead of duplicating it. with_pin_lock <- function(board, pin, expr) { + # `cache` is a pins implementation detail (verified against pins 1.4.x); + # the guards below fail open to an unlocked read if it ever goes away. cache <- board$cache # Boards without a download cache (e.g. board_folder) never download, so # there is no race to guard against. diff --git a/pkg-r/man/commons_prewarm.Rd b/pkg-r/man/commons_prewarm.Rd index 92e5b30a..03121c33 100644 --- a/pkg-r/man/commons_prewarm.Rd +++ b/pkg-r/man/commons_prewarm.Rd @@ -52,7 +52,10 @@ app reads the warmed cache. function to defer warming to post-startup idle time, so it happens while the user reads the welcome message. Outside a running Shiny app (e.g. a pre-deploy warm-up script) there is no \code{\link[later:later]{later::later()}} event loop, so -\code{commons_prewarm()} warms synchronously instead. +\code{commons_prewarm()} warms synchronously instead. Note that +\code{commons_prewarm()} always downgrades failures to warnings (see below), +even on this synchronous path — a pre-deploy script that should fail the +deploy on a cold cache must call \code{agent$prewarm()} directly. \code{prewarm()} lets failures propagate, since a direct call is typically warming caches ahead of deployment and a mere warning would sail through diff --git a/pkg-r/tests/testthat/test-context-layer.R b/pkg-r/tests/testthat/test-context-layer.R index 5d96d372..9ca5394d 100644 --- a/pkg-r/tests/testthat/test-context-layer.R +++ b/pkg-r/tests/testthat/test-context-layer.R @@ -202,6 +202,44 @@ test_that("prune_context_cache() keeps a single store larger than the cap", { expect_true(file.exists(big)) }) +test_that("prune_context_cache() reaps stale .build-* temp files only", { + dir <- withr::local_tempdir() + old <- file.path(dir, ".build-old") + fresh <- file.path(dir, ".build-fresh") + store <- file.path(dir, "store.duckdb") + for (f in c(old, fresh, store)) { + writeLines("x", f) + } + Sys.setFileTime(old, Sys.time() - 25 * 60 * 60) + + context_cache_state$n_builds <- 0 + context_cache_state$last_prune <- NULL + prune_context_cache(dir) + + expect_false(file.exists(old)) + expect_true(file.exists(fresh)) + expect_true(file.exists(store)) +}) + +test_that("context_store() warns and rebuilds when the cached store won't open", { + layer <- new_context_layer(c("Some context about widgets.")) + path <- context_store_path(context_layer_state(layer)$docs) + dir.create(dirname(path), recursive = TRUE, showWarnings = FALSE) + # Stand in for a store unlinked or corrupted between file.exists() and + # connect (e.g. by a concurrent pruner) + writeLines("not a duckdb file", path) + + expect_warning( + store <- context_store(layer), + "rebuilding" + ) + expect_identical(store, context_layer_state(layer)$store) + expect_equal( + context_search(layer, "widgets"), + "Some context about widgets." + ) +}) + test_that("cache root prefers the option, then env vars", { withr::local_options(commons.context_cache = NULL) withr::local_envvar( From 6b31dd7edba1f44776cc1a2b4ea66dd7e3320ad9 Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 31 Aug 2026 18:02:46 -0500 Subject: [PATCH 08/11] Harden prewarm warning and cache dir probing - commons_prewarm(): interpolate the error message safely so braces in raw error text (e.g. DuckDB's embedded JSON) can't throw inside the handler and escape the later::later() callback. - context_cache_dir_safe(): probe writability by creating and deleting a temp file instead of file.access(), which checks DOS attributes rather than ACLs on Windows. - reap_stale_build_files(): drop NA mtimes from files deleted by a concurrent process mid-call. - Document with_pin_lock()'s lock-name collision and lock-file litter. --- pkg-r/R/chat.R | 8 +++++++- pkg-r/R/context-layer.R | 12 ++++++++++-- pkg-r/R/data-source.R | 3 +++ pkg-r/tests/testthat/test-chat.R | 13 +++++++++++++ 4 files changed, 33 insertions(+), 3 deletions(-) diff --git a/pkg-r/R/chat.R b/pkg-r/R/chat.R index dbd35a74..0161450d 100644 --- a/pkg-r/R/chat.R +++ b/pkg-r/R/chat.R @@ -197,7 +197,13 @@ commons_prewarm <- function(client) { warm <- function() { tryCatch( client$prewarm(), - error = function(err) cli::cli_warn(conditionMessage(err)) + error = function(err) { + # Assign first: the raw message can contain braces (DuckDB errors + # embed JSON), which cli would try to interpolate -- and an error + # escaping this handler would stop the app. + msg <- conditionMessage(err) + cli::cli_warn("{msg}") + } ) } # later::later() only fires while an event loop is running; outside Shiny diff --git a/pkg-r/R/context-layer.R b/pkg-r/R/context-layer.R index f8570e87..9a5c1406 100644 --- a/pkg-r/R/context-layer.R +++ b/pkg-r/R/context-layer.R @@ -306,7 +306,9 @@ reap_stale_build_files <- function(dir, now, max_age = 24 * 60 * 60) { all.files = TRUE, full.names = TRUE ) - old <- stale[difftime(now, file.mtime(stale), units = "secs") > max_age] + age <- difftime(now, file.mtime(stale), units = "secs") + # file.mtime() is NA for a file a concurrent process just deleted + old <- stale[!is.na(age) & age > max_age] suppressWarnings(unlink(old, recursive = TRUE)) invisible() } @@ -383,10 +385,16 @@ is_hosted_shiny_app <- function() { # store becomes per-process, as if persistence were disabled). context_cache_dir_safe <- function() { dir <- context_cache_dir() + # Probe with an actual write: file.access() checks DOS attributes rather + # than ACLs on Windows, so it can report an unwritable dir as writable. ok <- tryCatch( { dir.create(dir, recursive = TRUE, showWarnings = FALSE) - dir.exists(dir) && file.access(dir, 2) == 0 + # tempfile() warns (not errors) when dir isn't a directory + dir.exists(dir) && { + probe <- tempfile(tmpdir = dir) + file.create(probe) && unlink(probe) == 0 + } }, error = function(err) FALSE ) diff --git a/pkg-r/R/data-source.R b/pkg-r/R/data-source.R index 66044516..c168621a 100644 --- a/pkg-r/R/data-source.R +++ b/pkg-r/R/data-source.R @@ -501,6 +501,9 @@ with_pin_lock <- function(board, pin, expr) { if (is.null(cache) || is.na(cache) || !nzchar(cache)) { return(force(expr)) } + # Sanitized names can collide ("a/b" vs "a_b"), which merely serializes + # two pins on one lock. Lock files are never removed, but they're empty + # and there is at most one per pin. name <- gsub("[^A-Za-z0-9._-]", "_", pin) dir.create(cache, recursive = TRUE, showWarnings = FALSE) lock <- filelock::lock(file.path(cache, paste0("commons-", name, ".lock"))) diff --git a/pkg-r/tests/testthat/test-chat.R b/pkg-r/tests/testthat/test-chat.R index 89aade80..832347ce 100644 --- a/pkg-r/tests/testthat/test-chat.R +++ b/pkg-r/tests/testthat/test-chat.R @@ -119,6 +119,19 @@ test_that("commons_prewarm() downgrades prewarm failures to warnings", { expect_warning(commons_prewarm(agent), "index build exploded") }) +test_that("commons_prewarm() warns on failures with braces in the message", { + path <- withr::local_tempfile(fileext = ".md") + writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) + agent <- test_agent(context_layer = context_layer(files = path)) + + # DuckDB errors embed JSON; cli must not interpolate the raw message + local_mocked_bindings( + context_store = function(...) stop('bad store: {"code": 1}'), + .package = "commons" + ) + expect_warning(commons_prewarm(agent), "bad store", fixed = TRUE) +}) + test_that("commons_app() prewarms the agent on idle", { skip_if_not_installed("shiny") skip_if_not_installed("shinychat") From 19bbb24572fa7ed6f461c45d767480bd50eeb186 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 28 Aug 2026 14:59:35 -0500 Subject: [PATCH 09/11] Query parquet/csv pins in place via DuckDB views First use of a pinned table used to pin_read() the data into R and dbWriteTable() a copy into DuckDB. For the formats DuckDB reads natively with built-in table functions (parquet, csv), first use now registers a view over the pin's versioned file instead: no copy, no R round-trip, and queries get column and predicate pushdown. The view references the resolved version's path, so it is as stable as the version itself. The locked-down connection gains an allowed_directories exception scoped to exactly the board's pin root (its cache for remote boards, its directory for folder boards), set before external access is disabled -- pin files become the only files the agent's queries can read. Extension readers (e.g. read_json_auto) stay off the view path because autoload is disabled; json/rds/qs2/arrow pins keep the eager path. A view dangles if its file disappears after registration (a rewrite on a non-versioned board deletes the old version's directory, or the cache is pruned). source_query()'s error-driven retry loop and source_describe()'s sample now re-resolve the latest version and re-register -- view again, or an eager load if the format changed -- before surfacing an error. --- pkg-r/R/data-source.R | 232 +++++++++++++++++++----- pkg-r/man/data_source.Rd | 25 ++- pkg-r/tests/testthat/test-data-source.R | 66 +++++++ 3 files changed, 272 insertions(+), 51 deletions(-) diff --git a/pkg-r/R/data-source.R b/pkg-r/R/data-source.R index c168621a..544156fd 100644 --- a/pkg-r/R/data-source.R +++ b/pkg-r/R/data-source.R @@ -12,16 +12,21 @@ #' * A `pins` board, e.g. [pins::board_connect()], is read into the same #' in-process database: each pin in `tables` becomes a table. Pin names are #' validated against the board at construction (a single listing call), but -#' each pin is downloaded only when its table is first used. Calling the -#' agent's `prewarm_sources()` method (see [commons_prewarm()]) starts a -#' background process that downloads the remaining pins into the local -#' pins cache, so a first use typically only reads an already-downloaded -#' file. Since the pins cache is on disk, `prewarm_sources()` can also run -#' ahead of deployment to warm the cache the deployed app will read. A -#' table reflects the pin's value at first use and is not refreshed for -#' the lifetime of the data source; if a pin can't be read (e.g. a network -#' failure), the error surfaces at that first use and the read is retried -#' on the next one. +#' each pin is downloaded only when its table is first used. Parquet and +#' CSV pins are then queried in place -- a DuckDB view over the downloaded +#' file, with no copy into the database -- while other formats (e.g. RDS) +#' are read and loaded at first use. Calling the agent's +#' `prewarm_sources()` method (see [commons_prewarm()]) starts a background +#' process that downloads the remaining pins into the local pins cache, so +#' a first use typically only reads an already-downloaded file. Since the +#' pins cache is on disk, `prewarm_sources()` can also run ahead of +#' deployment to warm the cache the deployed app will read. A table +#' reflects the pin version resolved at first use and is not refreshed for +#' the lifetime of the data source; if the board deletes that version +#' (e.g. a rewrite on a non-versioned board), the next query re-resolves +#' the latest version. If a pin can't be read (e.g. a network failure), +#' the error surfaces at that first use and the read is retried on the +#' next one. #' #' @param ... A single DBI connection, a single `pins` board, or named data #' frames to register as tables. When passing data frames, each name becomes @@ -333,9 +338,12 @@ data_source_board <- function( # Lock the connection down before any writes; lock_configuration() only # freezes SET statements, so later dbWriteTable() from a deferred read still - # works. + # works. Views over pin files read straight from the board's directory + # (folder boards) or local cache (remote boards), so allowlist exactly that + # root: the connection can read pin files and nothing else. + root <- board_pin_root(board) con <- duckdb_connect() - duckdb_lock_down(con) + duckdb_lock_down(con, allow_dirs = root) check_labels_free(con, names(tables), call = call) new_data_source( @@ -343,19 +351,36 @@ data_source_board <- function( names(tables), owned = TRUE, dictionary = dictionary, - pending = new_pending_pins(board, tables) + pending = new_pending_pins(board, tables, root) ) } +# The directory pin files for this board live under: the local cache for +# remote boards (connect, s3, ...), the board's own directory for folder +# boards. NULL when the board's layout is unknown, which disables the +# zero-copy view path (pins load eagerly instead). +board_pin_root <- function(board) { + for (field in c("cache", "path")) { + val <- board[[field]] + if (!is.null(val) && !is.na(val) && nzchar(val)) { + return(val) + } + } + NULL +} + # The deferred-read state a board source carries: the board plus the pins not # yet loaded (named character: table label -> pin name). Shared by every alias # of the source, so a read through one is seen by all. source_prewarm() also # stores its background downloader's handle here ($process), so all aliases see # at most one live warmer. -new_pending_pins <- function(board, tables) { +new_pending_pins <- function(board, tables, root = NULL) { pending <- new.env(parent = emptyenv()) pending$board <- board pending$pins <- tables + pending$root <- root + # Tables registered as views over pin files: table label -> list(pin, path). + pending$views <- list() pending } @@ -457,9 +482,16 @@ source_ensure_tables <- function(source, tables, call = rlang::caller_env()) { ) for (table in todo) { pin <- pending$pins[[table]] - value <- tryCatch( - with_pin_lock(pending$board, pin, pins::pin_read(pending$board, pin)), + tryCatch( + with_pin_lock( + pending$board, + pin, + source_load_pin(source, table, pin, call = call) + ), error = function(err) { + if (inherits(err, "commons_pin_not_data_frame")) { + stop(err) + } cli::cli_abort( "Failed to read pin {.val {pin}} for table {.val {table}}.", parent = err, @@ -467,21 +499,109 @@ source_ensure_tables <- function(source, tables, call = rlang::caller_env()) { ) } ) - if (!is.data.frame(value)) { - cli::cli_abort( - c( - "Pin {.val {pin}} for table {.val {table}} is not a data frame.", - i = "It is {.obj_type_friendly {value}}." - ), - call = call - ) - } - DBI::dbWriteTable(state$con, table, as.data.frame(value), overwrite = TRUE) pending$pins <- pending$pins[setdiff(names(pending$pins), table)] } invisible(source) } +# Register a pinned table in DuckDB. When the pin is a single file DuckDB +# reads natively (parquet/csv/json), register a view over the downloaded +# file instead of copying the data in: the load is free, the data stays in +# the pins cache, and queries get DuckDB's column and predicate pushdown. +# The view references the resolved version's file, so it is as stable as +# that version; refresh_dangling_views() covers boards that delete old +# versions. Other formats (rds, qs2, arrow, uploads) take the eager path. +source_load_pin <- function(source, table, pin, call = rlang::caller_env()) { + state <- data_source_state(source) + board <- state$pending$board + meta <- pins::pin_meta(board, pin) + version <- meta$local$version + path <- pins::pin_download(board, pin, version = version) + + reader <- if (length(path) == 1 && !is.null(state$pending$root)) { + pin_view_reader(meta$type) + } + if (!is.null(reader)) { + create_pin_view(state$con, table, path, reader) + state$pending$views[[table]] <- list(pin = pin, path = path) + return(invisible(source)) + } + + value <- pins::pin_read(board, pin, version = version) + if (!is.data.frame(value)) { + cli::cli_abort( + c( + "Pin {.val {pin}} for table {.val {table}} is not a data frame.", + i = "It is {.obj_type_friendly {value}}." + ), + class = "commons_pin_not_data_frame", + call = call + ) + } + # A previous registration may have been a view (e.g. the pin's format + # changed across versions); dbWriteTable can't overwrite a view. + DBI::dbExecute( + state$con, + sprintf("DROP VIEW IF EXISTS %s", DBI::dbQuoteIdentifier(state$con, table)) + ) + DBI::dbWriteTable(state$con, table, as.data.frame(value), overwrite = TRUE) + invisible(source) +} + +# Only formats DuckDB reads with built-in (non-extension) table functions: +# the connection is locked down with extension autoload disabled, so +# extension-backed readers (e.g. read_json_auto) are unavailable. +pin_view_reader <- function(type) { + switch( + type, + parquet = "read_parquet", + csv = "read_csv_auto" + ) +} + +create_pin_view <- function(con, table, path, reader) { + DBI::dbExecute( + con, + sprintf( + "CREATE OR REPLACE VIEW %s AS SELECT * FROM %s(%s)", + DBI::dbQuoteIdentifier(con, table), + reader, + DBI::dbQuoteString(con, path) + ) + ) +} + +# A view over a pin file dangles when the file disappears after registration: +# a rewrite on a non-versioned board deletes the old version's directory, and +# cache pruning can remove files too. Re-resolve the pin's latest version and +# register it afresh (a view again, or an eager load if the format changed). +# Returns TRUE only when a view was actually refreshed, so callers can retry +# the failed query once without risking an unbounded loop. +refresh_dangling_views <- function(source) { + pending <- data_source_state(source)$pending + if (is.null(pending) || length(pending$views) == 0) { + return(FALSE) + } + refreshed <- FALSE + for (table in names(pending$views)) { + view <- pending$views[[table]] + if (file.exists(view$path)) { + next + } + tryCatch( + { + with_pin_lock(pending$board, view$pin, { + pending$views[[table]] <- NULL + source_load_pin(source, table, view$pin) + }) + refreshed <- TRUE + }, + error = function(err) NULL + ) + } + refreshed +} + source_ensure_all <- function(source, call = rlang::caller_env()) { state <- data_source_state(source) source_ensure_tables(source, state$tables, call = call) @@ -617,13 +737,20 @@ source_describe <- function( catalog_ensure_queryable(source, table, call = call) source_ensure_tables(source, table) - sample <- DBI::dbGetQuery( - state$con, - sprintf( - "SELECT * FROM %s LIMIT %d", - DBI::dbQuoteIdentifier(state$con, id), - n_sample - ) + sample_sql <- sprintf( + "SELECT * FROM %s LIMIT %d", + DBI::dbQuoteIdentifier(state$con, id), + n_sample + ) + sample <- tryCatch( + DBI::dbGetQuery(state$con, sample_sql), + error = function(err) { + if (refresh_dangling_views(source)) { + DBI::dbGetQuery(state$con, sample_sql) + } else { + stop(err) + } + } ) relation <- source_relation(source, table) if (is.null(state$relations)) { @@ -721,10 +848,14 @@ source_query <- function(source, sql) { return(result) } todo <- pending_tables_in_error(source, result) - if (length(todo) == 0) { - stop(result) + if (length(todo) > 0) { + source_ensure_tables(source, todo) + next } - source_ensure_tables(source, todo) + if (refresh_dangling_views(source)) { + next + } + stop(result) } } @@ -803,18 +934,37 @@ check_query <- function(sql, call = rlang::caller_env()) { # DuckDB-specific hardening for the connection we own: no extension loading, # no filesystem or external access, and the configuration locked thereafter. -duckdb_lock_down <- function(con) { +duckdb_lock_down <- function(con, allow_dirs = character(0)) { + # allowed_directories must be set while external access is still enabled, + # and is incompatible with disabling LocalFileSystem (the filesystem-level + # block wins over the path allowlist). With an allowlist, external access + # stays disabled and only the listed directories remain readable. + if (length(allow_dirs) > 0) { + DBI::dbExecute( + con, + sprintf( + "SET allowed_directories = [%s]", + paste(DBI::dbQuoteString(con, allow_dirs), collapse = ", ") + ) + ) + } DBI::dbExecute( con, - " + sprintf( + " SET allow_community_extensions = false; SET allow_unsigned_extensions = false; SET autoinstall_known_extensions = false; SET autoload_known_extensions = false; SET enable_external_access = false; -SET disabled_filesystems = 'LocalFileSystem'; -SET lock_configuration = true; -" +%sSET lock_configuration = true; +", + if (length(allow_dirs) == 0) { + "SET disabled_filesystems = 'LocalFileSystem';\n" + } else { + "" + } + ) ) invisible(con) } diff --git a/pkg-r/man/data_source.Rd b/pkg-r/man/data_source.Rd index 390bfc6e..2fbbdfa1 100644 --- a/pkg-r/man/data_source.Rd +++ b/pkg-r/man/data_source.Rd @@ -61,16 +61,21 @@ when the data isn't already in a database. \item A \code{pins} board, e.g. \code{\link[pins:board_connect]{pins::board_connect()}}, is read into the same in-process database: each pin in \code{tables} becomes a table. Pin names are validated against the board at construction (a single listing call), but -each pin is downloaded only when its table is first used. Calling the -agent's \code{prewarm_sources()} method (see \code{\link[=commons_prewarm]{commons_prewarm()}}) starts a -background process that downloads the remaining pins into the local -pins cache, so a first use typically only reads an already-downloaded -file. Since the pins cache is on disk, \code{prewarm_sources()} can also run -ahead of deployment to warm the cache the deployed app will read. A -table reflects the pin's value at first use and is not refreshed for -the lifetime of the data source; if a pin can't be read (e.g. a network -failure), the error surfaces at that first use and the read is retried -on the next one. +each pin is downloaded only when its table is first used. Parquet and +CSV pins are then queried in place -- a DuckDB view over the downloaded +file, with no copy into the database -- while other formats (e.g. RDS) +are read and loaded at first use. Calling the agent's +\code{prewarm_sources()} method (see \code{\link[=commons_prewarm]{commons_prewarm()}}) starts a background +process that downloads the remaining pins into the local pins cache, so +a first use typically only reads an already-downloaded file. Since the +pins cache is on disk, \code{prewarm_sources()} can also run ahead of +deployment to warm the cache the deployed app will read. A table +reflects the pin version resolved at first use and is not refreshed for +the lifetime of the data source; if the board deletes that version +(e.g. a rewrite on a non-versioned board), the next query re-resolves +the latest version. If a pin can't be read (e.g. a network failure), +the error surfaces at that first use and the read is retried on the +next one. } } \section{Data dictionaries}{ diff --git a/pkg-r/tests/testthat/test-data-source.R b/pkg-r/tests/testthat/test-data-source.R index 361a81a7..e3dbfbf9 100644 --- a/pkg-r/tests/testthat/test-data-source.R +++ b/pkg-r/tests/testthat/test-data-source.R @@ -498,3 +498,69 @@ test_that("as_data_sources validates its input", { error = TRUE ) }) + +test_that("parquet/csv pins register as zero-copy views over the pins cache", { + skip_if_not_installed("pins") + + board <- pins::board_temp() + suppressMessages({ + pins::pin_write(board, data.frame(id = 1:3, v = letters[1:3]), "p-parquet", type = "parquet") + pins::pin_write(board, data.frame(id = 4:6, v = letters[4:6]), "p-csv", type = "csv") + }) + src <- data_source( + board, + tables = c(parquet_t = "p-parquet", csv_t = "p-csv") + ) + + for (table in c("parquet_t", "csv_t")) { + res <- source_query(src, sprintf("SELECT * FROM %s", table)) + expect_equal(nrow(res), 3) + # Registered as a view over the cache file, not a copied table + catalog <- DBI::dbGetQuery( + src$con, + "SELECT table_type FROM information_schema.tables WHERE table_name = $1", + params = list(table) + ) + expect_equal(catalog$table_type, "VIEW") + # The view reads the pin's versioned file directly + expect_true(file.exists(src$pending$views[[table]]$path)) + } +}) + +test_that("rds pins still load eagerly as tables", { + skip_if_not_installed("pins") + + board <- board_with_pins("team-orders" = data.frame(id = 1:3)) + src <- data_source(board, tables = c(orders = "team-orders")) + + res <- source_query(src, "SELECT * FROM orders") + expect_equal(nrow(res), 3) + catalog <- DBI::dbGetQuery( + src$con, + "SELECT table_type FROM information_schema.tables WHERE table_name = 'orders'" + ) + expect_equal(catalog$table_type, "BASE TABLE") +}) + +test_that("a dangling pin view is re-resolved and retried transparently", { + skip_if_not_installed("pins") + + board <- pins::board_folder(withr::local_tempdir(), versioned = FALSE) + suppressMessages( + pins::pin_write(board, data.frame(id = 1L), "orders", type = "parquet") + ) + src <- data_source(board, tables = c(orders = "orders")) + + expect_equal(source_query(src, "SELECT * FROM orders")$id, 1L) + view_path <- src$pending$views$orders$path + + # A rewrite on a non-versioned board deletes the old version's directory + suppressMessages( + pins::pin_write(board, data.frame(id = 1:2), "orders", type = "parquet") + ) + expect_false(file.exists(view_path)) + + # The next query re-resolves the latest version and succeeds + expect_equal(source_query(src, "SELECT * FROM orders")$id, 1:2) + expect_true(file.exists(src$pending$views$orders$path)) +}) From 537aef72b4d68b56165f25e99f30878f2dfb12f4 Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 31 Aug 2026 18:22:50 -0500 Subject: [PATCH 10/11] Review fixes: side-effect-free cache probing, test hygiene - The cache_hit span attribute in prewarm_context() probes the store path with the side-effect-free context_cache_dir() resolver (context_store_path() gains an injectable cache_dir), so recording telemetry cannot create directories or trigger the fallback warning. - Add local_context_cache_state() to snapshot/restore the package-level cache housekeeping state in tests that poke it. - Fix view tests to reach connection/pending state via data_source_state() (sources are R6 since #228). - Document the deliberate cache-before-path field order in board_pin_root(). --- pkg-r/R/commons.R | 10 +++++++--- pkg-r/R/context-layer.R | 7 +++++-- pkg-r/R/data-source.R | 9 +++++++-- pkg-r/tests/testthat/test-context-layer.R | 18 ++++++++++++++++++ pkg-r/tests/testthat/test-data-source.R | 11 ++++++----- 5 files changed, 43 insertions(+), 12 deletions(-) diff --git a/pkg-r/R/commons.R b/pkg-r/R/commons.R index a772f689..17769129 100644 --- a/pkg-r/R/commons.R +++ b/pkg-r/R/commons.R @@ -409,13 +409,17 @@ Commons <- R6::R6Class( "commons_context_prewarm", attributes = list( "commons.context.n_docs" = length(layer_state$docs), - # tryCatch: telemetry must not abort prewarming (resolving the - # cache dir can fail or warn on an unwritable root). + # Probe with the side-effect-free resolver so recording telemetry + # can't create directories or trigger the fallback warning, and + # tryCatch so a resolution failure can't abort prewarming. "commons.context.cache_hit" = !is.null(layer_state$store) || isTRUE(tryCatch( context_cache_enabled() && - file.exists(context_store_path(layer_state$docs)), + file.exists(context_store_path( + layer_state$docs, + cache_dir = context_cache_dir() + )), error = function(err) FALSE )) ) diff --git a/pkg-r/R/context-layer.R b/pkg-r/R/context-layer.R index 9a5c1406..4d34b526 100644 --- a/pkg-r/R/context-layer.R +++ b/pkg-r/R/context-layer.R @@ -321,13 +321,16 @@ format_size <- function(bytes) { } } -context_store_path <- function(docs) { +# `cache_dir` is injectable so read-only probes (e.g. the cache_hit span +# attribute in prewarm_context()) can pass context_cache_dir() and avoid the +# fallback tempdir's side effects (directory creation, one-time warning). +context_store_path <- function(docs, cache_dir = context_cache_dir_safe()) { key <- rlang::hash(c( docs, paste0("ragnar:", utils::packageVersion("ragnar")), paste0("duckdb:", utils::packageVersion("duckdb")) )) - file.path(context_cache_dir_safe(), "context", paste0(key, ".duckdb")) + file.path(cache_dir, "context", paste0(key, ".duckdb")) } # Cache root resolution: an explicit override, then Connect's persistent diff --git a/pkg-r/R/data-source.R b/pkg-r/R/data-source.R index 544156fd..a4dda4c7 100644 --- a/pkg-r/R/data-source.R +++ b/pkg-r/R/data-source.R @@ -358,7 +358,9 @@ data_source_board <- function( # The directory pin files for this board live under: the local cache for # remote boards (connect, s3, ...), the board's own directory for folder # boards. NULL when the board's layout is unknown, which disables the -# zero-copy view path (pins load eagerly instead). +# zero-copy view path (pins load eagerly instead). The field order matters: +# `cache` wins because remote boards keep downloaded files there, while +# folder boards have no cache and only set `path`. board_pin_root <- function(board) { for (field in c("cache", "path")) { val <- board[[field]] @@ -611,7 +613,10 @@ source_ensure_all <- function(source, call = rlang::caller_env()) { # race a first-use pin_read() of the same pin and leave a truncated cache # entry that poisons later reads. Both sides take an exclusive lock keyed by # the board's cache path and pin name, making the cache single-writer: the -# reader waits out an in-flight download instead of duplicating it. +# reader waits out an in-flight download instead of duplicating it. Lock +# files are left in the cache after unlock: unlinking one while another +# process waits on it would break the mutual exclusion, and they cost one +# tiny file per pin. with_pin_lock <- function(board, pin, expr) { # `cache` is a pins implementation detail (verified against pins 1.4.x); # the guards below fail open to an unlocked read if it ever goes away. diff --git a/pkg-r/tests/testthat/test-context-layer.R b/pkg-r/tests/testthat/test-context-layer.R index 9ca5394d..a13eea2a 100644 --- a/pkg-r/tests/testthat/test-context-layer.R +++ b/pkg-r/tests/testthat/test-context-layer.R @@ -115,12 +115,26 @@ test_that("commons.context_cache = FALSE builds the store in memory", { ) }) +# Reset the package-level cache housekeeping state on test exit, so tests +# that poke it don't leak order-dependence into later tests. +local_context_cache_state <- function(env = parent.frame()) { + old <- as.list(context_cache_state) + withr::defer( + { + rm(list = ls(context_cache_state), envir = context_cache_state) + list2env(old, envir = context_cache_state) + }, + env + ) +} + test_that("an unwritable cache dir warns once and falls back to a tempdir", { # A file where the cache dir should be makes dir.create() fail. blocker <- withr::local_tempfile() writeLines("occupied", blocker) withr::local_options(commons.context_cache = file.path(blocker, "cache")) + local_context_cache_state() context_cache_state$warned <- NULL context_cache_state$fallback_dir <- NULL @@ -143,6 +157,7 @@ test_that("prune_context_cache() is throttled across builds", { writeLines(strrep("x", 1000), stale) # A prune just happened, and the build count isn't at a multiple of 20 + local_context_cache_state() context_cache_state$n_builds <- 1 context_cache_state$last_prune <- Sys.time() prune_context_cache(dir, max_size = 1) @@ -167,6 +182,7 @@ test_that("prune_context_cache() evicts least-recently-used stores over the size Sys.setFileTime(middle, now - 200) Sys.setFileTime(newest, now - 100) + local_context_cache_state() context_cache_state$n_builds <- 0 context_cache_state$last_prune <- NULL # Cap fits two stores; the oldest is evicted @@ -183,6 +199,7 @@ test_that("prune_context_cache() keeps a single store larger than the cap", { big <- file.path(dir, "big.duckdb") writeLines(strrep("x", 10000), big) + local_context_cache_state() context_cache_state$n_builds <- 0 context_cache_state$last_prune <- NULL context_cache_state$warned_size <- NULL @@ -212,6 +229,7 @@ test_that("prune_context_cache() reaps stale .build-* temp files only", { } Sys.setFileTime(old, Sys.time() - 25 * 60 * 60) + local_context_cache_state() context_cache_state$n_builds <- 0 context_cache_state$last_prune <- NULL prune_context_cache(dir) diff --git a/pkg-r/tests/testthat/test-data-source.R b/pkg-r/tests/testthat/test-data-source.R index e3dbfbf9..5826b908 100644 --- a/pkg-r/tests/testthat/test-data-source.R +++ b/pkg-r/tests/testthat/test-data-source.R @@ -512,18 +512,19 @@ test_that("parquet/csv pins register as zero-copy views over the pins cache", { tables = c(parquet_t = "p-parquet", csv_t = "p-csv") ) + state <- data_source_state(src) for (table in c("parquet_t", "csv_t")) { res <- source_query(src, sprintf("SELECT * FROM %s", table)) expect_equal(nrow(res), 3) # Registered as a view over the cache file, not a copied table catalog <- DBI::dbGetQuery( - src$con, + state$con, "SELECT table_type FROM information_schema.tables WHERE table_name = $1", params = list(table) ) expect_equal(catalog$table_type, "VIEW") # The view reads the pin's versioned file directly - expect_true(file.exists(src$pending$views[[table]]$path)) + expect_true(file.exists(state$pending$views[[table]]$path)) } }) @@ -536,7 +537,7 @@ test_that("rds pins still load eagerly as tables", { res <- source_query(src, "SELECT * FROM orders") expect_equal(nrow(res), 3) catalog <- DBI::dbGetQuery( - src$con, + data_source_state(src)$con, "SELECT table_type FROM information_schema.tables WHERE table_name = 'orders'" ) expect_equal(catalog$table_type, "BASE TABLE") @@ -552,7 +553,7 @@ test_that("a dangling pin view is re-resolved and retried transparently", { src <- data_source(board, tables = c(orders = "orders")) expect_equal(source_query(src, "SELECT * FROM orders")$id, 1L) - view_path <- src$pending$views$orders$path + view_path <- data_source_state(src)$pending$views$orders$path # A rewrite on a non-versioned board deletes the old version's directory suppressMessages( @@ -562,5 +563,5 @@ test_that("a dangling pin view is re-resolved and retried transparently", { # The next query re-resolves the latest version and succeeds expect_equal(source_query(src, "SELECT * FROM orders")$id, 1:2) - expect_true(file.exists(src$pending$views$orders$path)) + expect_true(file.exists(data_source_state(src)$pending$views$orders$path)) }) From dd59db3672e6544b4b00ad5abb0d53107519b5c3 Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 31 Aug 2026 18:40:29 -0500 Subject: [PATCH 11/11] Add nanoparquet to Suggests for the parquet pin tests --- pkg-r/DESCRIPTION | 1 + pkg-r/tests/testthat/test-data-source.R | 2 ++ 2 files changed, 3 insertions(+) diff --git a/pkg-r/DESCRIPTION b/pkg-r/DESCRIPTION index ea520960..67a487c6 100644 --- a/pkg-r/DESCRIPTION +++ b/pkg-r/DESCRIPTION @@ -53,6 +53,7 @@ Suggests: dplyr, ggplot2, gt, + nanoparquet, odbc, otel (>= 0.2.0), otelsdk (>= 0.2.0), diff --git a/pkg-r/tests/testthat/test-data-source.R b/pkg-r/tests/testthat/test-data-source.R index 5826b908..ab6dc471 100644 --- a/pkg-r/tests/testthat/test-data-source.R +++ b/pkg-r/tests/testthat/test-data-source.R @@ -501,6 +501,7 @@ test_that("as_data_sources validates its input", { test_that("parquet/csv pins register as zero-copy views over the pins cache", { skip_if_not_installed("pins") + skip_if_not_installed("nanoparquet") board <- pins::board_temp() suppressMessages({ @@ -545,6 +546,7 @@ test_that("rds pins still load eagerly as tables", { test_that("a dangling pin view is re-resolved and retried transparently", { skip_if_not_installed("pins") + skip_if_not_installed("nanoparquet") board <- pins::board_folder(withr::local_tempdir(), versioned = FALSE) suppressMessages(