From 676df17f06e2817d74e2f871c58ba43a31a1699e Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 28 Aug 2026 16:29:20 -0500 Subject: [PATCH 01/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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 1b6c8607d81af52c3782abe696e7d594da154c06 Mon Sep 17 00:00:00 2001 From: Carson Date: Tue, 1 Sep 2026 17:13:35 -0500 Subject: [PATCH 09/10] Harden the context cache pruner and writability probe - prune_context_cache() drops stores that stat as NA (deleted by a concurrent pruner mid-prune) instead of erroring on NA > max_size, and skips eviction victims that vanish before unlink. - context_cache_dir_safe() suppresses the file.create() probe warning so only the once-per-session cli warning surfaces. - Extract context_store_dir() so the cache's context/ layout has a single source of truth. --- pkg-r/R/context-layer.R | 75 +++++++++++----------- pkg-r/tests/testthat/setup-context-cache.R | 3 +- pkg-r/tests/testthat/test-context-layer.R | 73 ++++++++++++++++++--- 3 files changed, 102 insertions(+), 49 deletions(-) diff --git a/pkg-r/R/context-layer.R b/pkg-r/R/context-layer.R index 9a5c1406..5d659428 100644 --- a/pkg-r/R/context-layer.R +++ b/pkg-r/R/context-layer.R @@ -109,13 +109,9 @@ strip_frontmatter <- function(md) { # 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. -# Housekeeping state for the persistent context cache: prune throttling, the -# fallback tempdir, and the one-time warning about an unusable cache dir. +# cache root and later processes just open the file. Aliases of one layer +# share its store; augmenting its documents creates a layer with a fresh +# store. context_cache_state <- new.env(parent = emptyenv()) context_store <- function(layer) { @@ -132,8 +128,8 @@ context_store <- function(layer) { if (!file.exists(path)) { build_context_store(state$docs, path) } else { - # 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. + # Touch the mtime so size-cap eviction is LRU. Best-effort -- a + # read-only cache dir still opens fine. tryCatch(Sys.setFileTime(path, Sys.time()), error = function(err) NULL) } store <- tryCatch( @@ -144,8 +140,8 @@ context_store <- function(layer) { # 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. + # rebuild once -- a second failure propagates, so persistent corruption + # still surfaces. context_store_connect_warning(path, store) unlink(path) build_context_store(state$docs, path) @@ -175,9 +171,8 @@ context_store_connect_warning <- function(path, err) { } } -# 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. +# options(commons.context_cache = FALSE) builds the index in memory per +# layer instead -- an escape hatch for development loops over context files. context_cache_enabled <- function() { if (identical(getOption("commons.context_cache"), FALSE)) { return(FALSE) @@ -240,15 +235,12 @@ build_context_store <- function(docs, path) { } # 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. +# cache is capped by total size and pruned LRU (the mtime touch on open +# keeps active stores young). Like cachem, a single store larger than the +# cap is kept, with a one-time warning, rather than evicted into a rebuild +# loop. Throttled (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_size = getOption("commons.context_cache_max_size", 256 * 1024^2), @@ -268,15 +260,21 @@ prune_context_cache <- function( reap_stale_build_files(dir, now) stores <- list.files(dir, pattern = "[.]duckdb$", full.names = TRUE) - total <- sum(file.size(stores)) + # A store deleted by a concurrent pruner since list.files() stats as NA + sizes <- file.size(stores) + stores <- stores[!is.na(sizes)] + total <- sum(sizes, na.rm = TRUE) if (total > max_size) { evictable <- setdiff(stores, protect) - evictable <- evictable[order(file.mtime(evictable))] + evictable <- evictable[order(file.mtime(evictable), na.last = TRUE)] for (victim in evictable) { if (total <= max_size) { break } size <- file.size(victim) + if (is.na(size)) { + next + } # Only count the eviction when the unlink actually happened -- on # Windows, deleting a store another process holds open fails. if (suppressWarnings(unlink(victim)) == 0) { @@ -294,11 +292,9 @@ 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. +# A crashed or killed build would leak its `.build-*` temp file (the pruner +# only lists `*.duckdb`). Reap temp files older than a day: young enough to +# clear debris promptly, old enough to never delete a build in flight. reap_stale_build_files <- function(dir, now, max_age = 24 * 60 * 60) { stale <- list.files( dir, @@ -327,17 +323,17 @@ context_store_path <- function(docs) { paste0("ragnar:", utils::packageVersion("ragnar")), paste0("duckdb:", utils::packageVersion("duckdb")) )) - file.path(context_cache_dir_safe(), "context", paste0(key, ".duckdb")) + file.path(context_store_dir(), paste0(key, ".duckdb")) +} + +context_store_dir <- function(cache_dir = context_cache_dir_safe()) { + file.path(cache_dir, "context") } # Cache root resolution: an explicit override, then Connect's persistent -# 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. +# data directory, 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. context_cache_dir <- function() { opt <- getOption("commons.context_cache") if (!is.null(opt) && !identical(opt, FALSE)) { @@ -393,7 +389,8 @@ context_cache_dir_safe <- function() { # tempfile() warns (not errors) when dir isn't a directory dir.exists(dir) && { probe <- tempfile(tmpdir = dir) - file.create(probe) && unlink(probe) == 0 + # file.create() warns rather than errors on failure + suppressWarnings(file.create(probe)) && unlink(probe) == 0 } }, error = function(err) FALSE diff --git a/pkg-r/tests/testthat/setup-context-cache.R b/pkg-r/tests/testthat/setup-context-cache.R index 63922b97..79e222f7 100644 --- a/pkg-r/tests/testthat/setup-context-cache.R +++ b/pkg-r/tests/testthat/setup-context-cache.R @@ -1,5 +1,4 @@ -# 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. +# Isolate the persistent context store cache per test session. options(commons.context_cache = tempfile("commons-context-cache-")) withr::defer( unlink(getOption("commons.context_cache"), recursive = TRUE), diff --git a/pkg-r/tests/testthat/test-context-layer.R b/pkg-r/tests/testthat/test-context-layer.R index 9ca5394d..83f3d2d8 100644 --- a/pkg-r/tests/testthat/test-context-layer.R +++ b/pkg-r/tests/testthat/test-context-layer.R @@ -90,7 +90,7 @@ test_that("the context store persists on disk and is shared across layers", { 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(context_layer_state(layer2)$docs), @@ -98,7 +98,7 @@ test_that("the context store persists on disk and is shared across layers", { ) expect_match(context_search(layer2, "revenue")[[1]], "booked") - # Different docs key a different store + expect_false(identical(context_store_path("other docs"), store_path)) }) @@ -132,7 +132,7 @@ test_that("an unwritable cache dir warns once and falls back to a tempdir", { 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) }) @@ -142,13 +142,13 @@ test_that("prune_context_cache() is throttled across builds", { stale <- file.path(dir, "stale.duckdb") 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, 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, max_size = 1) expect_false(file.exists(stale)) @@ -169,7 +169,7 @@ test_that("prune_context_cache() evicts least-recently-used stores over the size 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) @@ -178,6 +178,63 @@ test_that("prune_context_cache() evicts least-recently-used stores over the size expect_true(file.exists(newest)) }) +test_that("an unwritable cache dir surfaces only the once-per-session warning", { + skip_on_os("windows") # Sys.chmod() read-only bits aren't enforced there + skip_if(.Platform$OS.type == "unix" && Sys.info()[["user"]] == "root") + + # The cache dir itself is unwritable, so the probe's file.create() fails + readonly <- withr::local_tempdir() + withr::defer(Sys.chmod(readonly, mode = "0755")) + Sys.chmod(readonly, mode = "0555") + withr::local_options(commons.context_cache = readonly) + + context_cache_state$warned <- NULL + context_cache_state$fallback_dir <- NULL + + + warnings <- character() + withCallingHandlers( + context_cache_dir_safe(), + warning = function(w) { + warnings <<- c(warnings, conditionMessage(w)) + invokeRestart("muffleWarning") + } + ) + expect_length(warnings, 1) + expect_match(warnings, "Falling back to a per-session temporary directory") + + warnings <- character() + withCallingHandlers( + context_cache_dir_safe(), + warning = function(w) { + warnings <<- c(warnings, conditionMessage(w)) + invokeRestart("muffleWarning") + } + ) + expect_length(warnings, 0) +}) + +test_that("prune_context_cache() tolerates stores vanishing mid-prune", { + dir <- withr::local_tempdir() + store <- file.path(dir, "store.duckdb") + writeLines(strrep("x", 1000), store) + + context_cache_state$n_builds <- 0 + context_cache_state$last_prune <- NULL + + # A broken symlink is listed but stats as NA, like a store a concurrent + # pruner deleted mid-prune + skip_on_os("windows") # symlinks need elevated privileges there + ghost <- file.path(dir, "ghost.duckdb") + file.symlink(file.path(dir, "no-such-target"), ghost) + + context_cache_state$warned_size <- NULL + expect_no_warning( + expect_no_error(prune_context_cache(dir, max_size = file.size(store))) + ) + expect_true(file.exists(store)) +}) + test_that("prune_context_cache() keeps a single store larger than the cap", { dir <- withr::local_tempdir() big <- file.path(dir, "big.duckdb") @@ -193,7 +250,7 @@ test_that("prune_context_cache() keeps a single store larger than the 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( @@ -271,7 +328,7 @@ 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 672fd59d76af82e66daf6e1fba8d77a9eeab84ed Mon Sep 17 00:00:00 2001 From: Carson Date: Tue, 1 Sep 2026 17:13:35 -0500 Subject: [PATCH 10/10] Refine commons_prewarm() as an interactive, pre-deploy entry point commons_prewarm() is now synchronous-only, for warming caches from a script ahead of deployment: it takes a cache_dir argument (scoped to the call) so the warmed context index can ship inside the app bundle, and announces where the cache landed with guidance on making it reachable from a deployment (including that rsconnect excludes app_cache/ from bundles). The Shiny idle-time path moved to an internal prewarm_on_idle() helper that owns the later::later() deferral and warning downgrade. The agent's public prewarm surface is now just prewarm(); prewarm_context() and prewarm_sources() are private. Docs rewritten for new users: what prewarming does, when commons_server() already handles it, when to configure the cache directory (ephemeral hosts like Connect Cloud, shipping a warm cache, dev loops), and the early-access status of Connect's persistent data directories. --- pkg-r/R/chat.R | 178 +++++++++++++++++----------- pkg-r/R/commons.R | 23 ++-- pkg-r/R/data-source.R | 10 +- pkg-r/man/commons_prewarm.Rd | 121 ++++++++++--------- pkg-r/man/data_source.Rd | 4 +- pkg-r/tests/testthat/test-chat.R | 46 ++++++- pkg-r/tests/testthat/test-commons.R | 31 ++--- pkg-r/vignettes/commons.Rmd | 2 +- 8 files changed, 244 insertions(+), 171 deletions(-) diff --git a/pkg-r/R/chat.R b/pkg-r/R/chat.R index 0161450d..42c6c810 100644 --- a/pkg-r/R/chat.R +++ b/pkg-r/R/chat.R @@ -111,7 +111,7 @@ commons_server <- function(id, client, ...) { attributes = list("commons.server.id" = id) ) - commons_prewarm(client) + prewarm_on_idle(client) chat <- shinychat::chat_server(id, client = client, ...) # shinychat owns the conversation identity (it sets the client's @@ -123,99 +123,139 @@ commons_server <- function(id, client, ...) { chat } -#' Pre-warm a commons agent during post-startup idle time +#' Pre-warm a commons agent's caches ahead of deployment #' -#' 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: +#' A [commons()] agent does some expensive setup the first time it needs to: +#' building the search index over the context layer and downloading any +#' uncached pins (see [data_source()]). When you serve the agent with +#' [commons_server()] or [commons_app()], this warming already happens +#' automatically during post-startup idle time, so the first question is +#' (hopefully) fast — you (hopefully) don't need to call +#' `commons_prewarm()` yourself. #' -#' * `agent$prewarm_context()` builds the context index (the store behind -#' `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, an `app_cache/commons` directory beside a Shiny app, or the -#' 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. 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 -#' deployment — outside the Shiny runtime entirely — and the deployed -#' app reads the warmed cache. +#' The reason to call it directly is to warm the caches *without* running +#' the app. Both kinds of setup are cached on disk — the context index is +#' built once per version of your context documents, and pins are +#' downloaded once into the local pins cache — so running +#' `commons_prewarm(agent, cache_dir)` in a script before deploying lets +#' the deployed app start warm. #' -#' `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. 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. 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. +#' Pre-warming is a pure optimization — anything it builds is rebuilt on +#' demand if it's missing — so failures are reported as warnings rather +#' than errors. If a pre-deploy script should fail the deploy when warming +#' fails, call the agent's `prewarm()` method directly instead; it lets +#' errors propagate. #' -#' `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. +#' @section Cache configuration: +#' You can usually ignore this section: by default the context index cache +#' lives in a per-user directory that does the right thing locally and on +#' most hosted platforms. The reasons to configure it are: +#' +#' * **Persistence across deployments on ephemeral hosts.** This is +#' already handled on Connect and Shiny Server: the cache automatically +#' lands in Connect's persistent data directory when the server provides +#' one (currently an early-access feature the administrator enables), or +#' in an `app_cache/` directory beside the app, which survives +#' redeploys. But on hosts where no local disk persists (e.g. Connect +#' Cloud, which resets disk to the deployed bundle and never sets a data +#' directory), the cache is rebuilt after every redeploy unless you +#' point it at persistent storage yourself. +#' * **Shipping a warm cache with the app.** Run +#' `commons_prewarm(agent, cache_dir = "path/inside/the/app")` before +#' deploying, and the deployed bundle includes the pre-built index. +#' `cache_dir` is required for this reason — anything resolved +#' implicitly (a per-user cache directory) would not ship with the +#' deployment. +#' * **Development loops.** If you're editing context documents and want +#' each change re-indexed from scratch, disable persistence with +#' `options(commons.context_cache = FALSE)`. +#' +#' Set the directory with `options(commons.context_cache = "path/to/dir")` +#' or the `COMMONS_CONTEXT_CACHE` environment variable. The cache is +#' capped at 256 MB with least-recently-used eviction; raise +#' `options(commons.context_cache_max_size)` (in bytes) if you index very +#' large context. #' #' @param client A [commons()] agent. +#' @param cache_dir A directory for the context index cache, used for this +#' call only (equivalent to setting +#' `options(commons.context_cache = cache_dir)` around it). Point it at +#' a directory inside the app so the warmed index ships with the +#' deployment. Note that rsconnect excludes `app_cache/` from deployed +#' bundles, so pick another name. #' #' @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) -#' } +#' # In a pre-deploy script: warm the caches into a directory inside the +#' # app, so the deployed bundle includes the pre-built context index +#' agent <- commons( +#' ellmer::chat_anthropic(), +#' data_sources = data_source(sales = sales) +#' ) +#' # (not app_cache/, which rsconnect excludes from the bundle) +#' commons_prewarm(agent, cache_dir = "commons-cache") #' } #' #' @export -commons_prewarm <- function(client) { +commons_prewarm <- function(client, cache_dir) { 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. - warm <- function() { + if (missing(cache_dir)) { + cli::cli_abort(c( + "{.arg cache_dir} is required.", + i = "Point it at a directory inside the app (e.g. {.code \"commons-cache\"}) so the warmed cache ships with the deployment." + )) + } + if (!rlang::is_string(cache_dir)) { + cli::cli_abort("{.arg cache_dir} must be a path to a cache directory.") + } + tryCatch( + { + withr::with_options(list(commons.context_cache = cache_dir), { + client$prewarm() + prewarm_cache_hint(cache_dir) + }) + }, + error = function(err) { + msg <- conditionMessage(err) + cli::cli_warn("{msg}") + } + ) + invisible(NULL) +} + +# An error escaping a later::later() callback would stop the app, so +# downgrade failures to warnings. +prewarm_on_idle <- function(client) { + later::later(function() { tryCatch( client$prewarm(), 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 - # (e.g. a pre-deploy warm-up script) the callback would never run. - if (is_shiny_app()) { - later::later(warm) - } else { - warm() - } + }) invisible(NULL) } +prewarm_cache_hint <- function(cache_dir) { + store_dir <- context_store_dir(cache_dir) + if (!dir.exists(store_dir)) { + cli::cli_warn(c( + "No context index was cached at {.path {cache_dir}}.", + i = "The agent has no context layer to index, so the deployed app has nothing to reuse." + )) + return(invisible()) + } + cli::cli_inform(c( + "Warmed the context index cache at {.path {store_dir}}.", + i = "To reuse it, deploy the directory with the app and set {.code options(commons.context_cache = \"{cache_dir}\")} in the app, or the {.envvar COMMONS_CONTEXT_CACHE} environment variable on the server." + )) + invisible() +} + 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 a772f689..beefcd72 100644 --- a/pkg-r/R/commons.R +++ b/pkg-r/R/commons.R @@ -390,17 +390,15 @@ 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. - self$prewarm_context() - self$prewarm_sources() + # A direct call is typically warming caches ahead of deployment, so + # failures propagate: a cold cache should fail the deploy. + # commons_prewarm() and prewarm_on_idle() downgrade them to warnings. + private$prewarm_context() + private$prewarm_sources() invisible(self) - }, - + } + ), + private = list( prewarm_context = function() { layer <- private$context_layer layer_state <- if (is.null(layer)) NULL else context_layer_state(layer) @@ -430,9 +428,8 @@ Commons <- R6::R6Class( source_prewarm(source) } invisible(self) - } - ), - private = list( + }, + sources = NULL, context_layer = NULL, registry = NULL, diff --git a/pkg-r/R/data-source.R b/pkg-r/R/data-source.R index c168621a..992cb55f 100644 --- a/pkg-r/R/data-source.R +++ b/pkg-r/R/data-source.R @@ -13,10 +13,10 @@ #' 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 +#' 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. Since the pins cache is on disk, `prewarm_sources()` can also run +#' file. Since the pins cache is on disk, `prewarm()` 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 @@ -489,9 +489,9 @@ source_ensure_all <- function(source, call = rlang::caller_env()) { # 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. +# entry. Both sides take an exclusive lock keyed by the board's cache path +# and pin name, so 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. diff --git a/pkg-r/man/commons_prewarm.Rd b/pkg-r/man/commons_prewarm.Rd index 03121c33..c87e9a88 100644 --- a/pkg-r/man/commons_prewarm.Rd +++ b/pkg-r/man/commons_prewarm.Rd @@ -2,78 +2,89 @@ % Please edit documentation in R/chat.R \name{commons_prewarm} \alias{commons_prewarm} -\title{Pre-warm a commons agent during post-startup idle time} +\title{Pre-warm a commons agent's caches ahead of deployment} \usage{ -commons_prewarm(client) +commons_prewarm(client, cache_dir) } \arguments{ \item{client}{A \code{\link[=commons]{commons()}} agent.} + +\item{cache_dir}{A directory for the context index cache, used for this +call only (equivalent to setting +\code{options(commons.context_cache = cache_dir)} around it). Point it at +a directory inside the app so the warmed index ships with the +deployment. Note that rsconnect excludes \verb{app_cache/} from deployed +bundles, so pick another name.} } \value{ \code{NULL}, invisibly. } \description{ -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: +A \code{\link[=commons]{commons()}} agent does some expensive setup the first time it needs to: +building the search index over the context layer and downloading any +uncached pins (see \code{\link[=data_source]{data_source()}}). When you serve the agent with +\code{\link[=commons_server]{commons_server()}} or \code{\link[=commons_app]{commons_app()}}, this warming already happens +automatically during post-startup idle time, so the first question is +(hopefully) fast — you (hopefully) don't need to call +\code{commons_prewarm()} yourself. } \details{ -\itemize{ -\item \code{agent$prewarm_context()} builds the context index (the store behind -\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, an \code{app_cache/commons} directory beside a Shiny app, or the -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. 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 -deployment — outside the Shiny runtime entirely — and the deployed -app reads the warmed cache. +The reason to call it directly is to warm the caches \emph{without} running +the app. Both kinds of setup are cached on disk — the context index is +built once per version of your context documents, and pins are +downloaded once into the local pins cache — so running +\code{commons_prewarm(agent, cache_dir)} in a script before deploying lets +the deployed app start warm. + +Pre-warming is a pure optimization — anything it builds is rebuilt on +demand if it's missing — so failures are reported as warnings rather +than errors. If a pre-deploy script should fail the deploy when warming +fails, call the agent's \code{prewarm()} method directly instead; it lets +errors propagate. } +\section{Cache configuration}{ -\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. 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. 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. +You can usually ignore this section: by default the context index cache +lives in a per-user directory that does the right thing locally and on +most hosted platforms. The reasons to configure it are: +\itemize{ +\item \strong{Persistence across deployments on ephemeral hosts.} This is +already handled on Connect and Shiny Server: the cache automatically +lands in Connect's persistent data directory when the server provides +one (currently an early-access feature the administrator enables), or +in an \verb{app_cache/} directory beside the app, which survives +redeploys. But on hosts where no local disk persists (e.g. Connect +Cloud, which resets disk to the deployed bundle and never sets a data +directory), the cache is rebuilt after every redeploy unless you +point it at persistent storage yourself. +\item \strong{Shipping a warm cache with the app.} Run +\code{commons_prewarm(agent, cache_dir = "path/inside/the/app")} before +deploying, and the deployed bundle includes the pre-built index. +\code{cache_dir} is required for this reason — anything resolved +implicitly (a per-user cache directory) would not ship with the +deployment. +\item \strong{Development loops.} If you're editing context documents and want +each change re-indexed from scratch, disable persistence with +\code{options(commons.context_cache = FALSE)}. +} -\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. +Set the directory with \code{options(commons.context_cache = "path/to/dir")} +or the \code{COMMONS_CONTEXT_CACHE} environment variable. The cache is +capped at 256 MB with least-recently-used eviction; raise +\code{options(commons.context_cache_max_size)} (in bytes) if you index very +large context. } + \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) -} +# In a pre-deploy script: warm the caches into a directory inside the +# app, so the deployed bundle includes the pre-built context index +agent <- commons( + ellmer::chat_anthropic(), + data_sources = data_source(sales = sales) +) +# (not app_cache/, which rsconnect excludes from the bundle) +commons_prewarm(agent, cache_dir = "commons-cache") } } diff --git a/pkg-r/man/data_source.Rd b/pkg-r/man/data_source.Rd index 390bfc6e..933945be 100644 --- a/pkg-r/man/data_source.Rd +++ b/pkg-r/man/data_source.Rd @@ -62,10 +62,10 @@ 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_sources()} method (see \code{\link[=commons_prewarm]{commons_prewarm()}}) starts a +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. Since the pins cache is on disk, \code{prewarm_sources()} can also run +file. Since the pins cache is on disk, \code{prewarm()} 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 diff --git a/pkg-r/tests/testthat/test-chat.R b/pkg-r/tests/testthat/test-chat.R index 832347ce..c266d1a6 100644 --- a/pkg-r/tests/testthat/test-chat.R +++ b/pkg-r/tests/testthat/test-chat.R @@ -115,8 +115,10 @@ test_that("commons_prewarm() downgrades prewarm failures to warnings", { context_store = function(...) stop("index build exploded"), .package = "commons" ) - # Outside a running Shiny app, commons_prewarm() warms synchronously. - expect_warning(commons_prewarm(agent), "index build exploded") + expect_warning( + commons_prewarm(agent, cache_dir = withr::local_tempdir()), + "index build exploded" + ) }) test_that("commons_prewarm() warns on failures with braces in the message", { @@ -129,9 +131,45 @@ test_that("commons_prewarm() warns on failures with braces in the message", { context_store = function(...) stop('bad store: {"code": 1}'), .package = "commons" ) - expect_warning(commons_prewarm(agent), "bad store", fixed = TRUE) + expect_warning( + commons_prewarm(agent, cache_dir = withr::local_tempdir()), + "bad store", + fixed = TRUE + ) +}) + +test_that("commons_prewarm(cache_dir =) builds the store in that directory", { + path <- withr::local_tempfile(fileext = ".md") + writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) + agent <- test_agent(context_layer = context_layer(files = path)) + + cache_dir <- withr::local_tempdir() + expect_message( + commons_prewarm(agent, cache_dir = cache_dir), + "Warmed the context index cache" + ) + expect_length(list.files(file.path(cache_dir, "context")), 1) }) +test_that("commons_prewarm() warns when there is nothing to cache", { + expect_warning( + commons_prewarm(test_agent(), cache_dir = withr::local_tempdir()), + "No context index was cached" + ) +}) + +test_that("commons_prewarm() validates cache_dir", { + expect_error( + commons_prewarm(test_agent(), cache_dir = TRUE), + "must be a path" + ) + expect_error( + commons_prewarm(test_agent()), + "cache_dir.* is required" + ) +}) + + test_that("commons_app() prewarms the agent on idle", { skip_if_not_installed("shiny") skip_if_not_installed("shinychat") @@ -140,7 +178,7 @@ test_that("commons_app() prewarms the agent on idle", { app_env <- environment(app$serverFuncSource) prewarmed <- FALSE testthat::local_mocked_bindings( - commons_prewarm = function(client) prewarmed <<- TRUE, + prewarm_on_idle = function(client) prewarmed <<- TRUE, .package = "commons" ) shiny::testServer(app_env$server, { diff --git a/pkg-r/tests/testthat/test-commons.R b/pkg-r/tests/testthat/test-commons.R index ec371cfe..22546238 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_context() builds the context store ahead of the first search", { +test_that("prewarm() 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,25 +428,13 @@ test_that("prewarm_context() builds the context store ahead of the first search" agent <- test_agent(context_layer = layer) expect_null(context_layer_state(layer)$store) - 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)) + expect_match(context_search(layer, "revenue")[[1]], "booked") }) 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", { @@ -459,10 +447,9 @@ 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_context() records a cache-miss build and its own span", { +test_that("prewarm() 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()) @@ -471,7 +458,7 @@ test_that("prewarm_context() records a cache-miss build and its own span", { writeLines(c("# Revenue", "", "Revenue means booked revenue."), path) agent <- test_agent(context_layer = context_layer(files = path)) - recorded <- otelsdk::with_otel_record(agent$prewarm_context()) + recorded <- otelsdk::with_otel_record(agent$prewarm()) names <- vapply(recorded$traces, `[[`, character(1), "name") expect_true("commons_context_store_build" %in% names) @@ -483,16 +470,16 @@ test_that("prewarm_context() records a cache-miss build and its own span", { expect_equal(prewarm_span$attributes[["commons.context.cache_hit"]], FALSE) }) -test_that("prewarm_context() records a cache hit without a build span", { +test_that("prewarm() 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) agent <- test_agent(context_layer = context_layer(files = path)) - agent$prewarm_context() + agent$prewarm() - recorded <- otelsdk::with_otel_record(agent$prewarm_context()) + recorded <- otelsdk::with_otel_record(agent$prewarm()) names <- vapply(recorded$traces, `[[`, character(1), "name") expect_false("commons_context_store_build" %in% names) @@ -500,7 +487,7 @@ test_that("prewarm_context() records a cache hit without a build span", { expect_equal(prewarm_span$attributes[["commons.context.cache_hit"]], TRUE) }) -test_that("prewarm_sources() warms board pins in the background without loading them", { +test_that("prewarm() warms board pins in the background without loading them", { skip_if_not_installed("pins") board <- board_with_pins( @@ -515,7 +502,7 @@ test_that("prewarm_sources() warms board pins in the background without loading expect_length(DBI::dbListTables(data_source_state(src)$con), 0) - agent$prewarm_sources() + agent$prewarm() 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 55016e40..661a9a5a 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. `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. +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; `agent$prewarm()` (see `commons_prewarm()`) warms the context index and pins cache ahead of deployment. This example assumes that `observations` and `site_area` are data frames loaded when the app starts.