Query parquet/csv pins in place via DuckDB views - #232
Draft
cpsievert wants to merge 11 commits into
Draft
Conversation
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.
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 (#214).
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
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.
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.
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.
… 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.
- 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.
First use of a pinned table used to pin_read() the data into R and dbWriteTable() a copy into DuckDB. For the formats DuckDB reads natively with built-in table functions (parquet, csv), first use now registers a view over the pin's versioned file instead: no copy, no R round-trip, and queries get column and predicate pushdown. The view references the resolved version's path, so it is as stable as the version itself. The locked-down connection gains an allowed_directories exception scoped to exactly the board's pin root (its cache for remote boards, its directory for folder boards), set before external access is disabled -- pin files become the only files the agent's queries can read. Extension readers (e.g. read_json_auto) stay off the view path because autoload is disabled; json/rds/qs2/arrow pins keep the eager path. A view dangles if its file disappears after registration (a rewrite on a non-versioned board deletes the old version's directory, or the cache is pruned). source_query()'s error-driven retry loop and source_describe()'s sample now re-resolve the latest version and re-register -- view again, or an eager load if the format changed -- before surfacing an error.
- The cache_hit span attribute in prewarm_context() probes the store path with the side-effect-free context_cache_dir() resolver (context_store_path() gains an injectable cache_dir), so recording telemetry cannot create directories or trigger the fallback warning. - Add local_context_cache_state() to snapshot/restore the package-level cache housekeeping state in tests that poke it. - Fix view tests to reach connection/pending state via data_source_state() (sources are R6 since #228). - Document the deliberate cache-before-path field order in board_pin_root().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes
Board-backed data sources previously loaded every pin eagerly into the in-process DuckDB: first use of a table meant
pin_read()into R followed bydbWriteTable()— a full copy of the data through R's memory for formats DuckDB could have read itself. This PR registers parquet and CSV pins as zero-copy views over the downloaded file in the pins cache instead. Formats DuckDB doesn't read with built-in table functions (RDS, qs2, arrow, uploads) keep the eager path unchanged.What this means in practice
CREATE VIEW(milliseconds) instead of a full read +dbWriteTable()copy.prewarm_sources()), first use is just view registration — there's nothing left to load in-process.The key ideas
Views are as stable as the pin version they point at. A table reflects the pin version resolved at first use, same as the eager path. When the underlying file disappears (a rewrite on a non-versioned board, cache pruning), the next failed query re-resolves the pin's latest version and retries once, transparently; a persistent failure surfaces the original error.
The lockdown relaxes to exactly one directory. The source's DuckDB connection stays locked down (no extensions, no filesystem access), but now allowlists the board's pin root via
allowed_directoriesso views can read pin files — and nothing else. Boards with an unknown layout skip the allowlist and load eagerly as before.The pins cache becomes single-writer. pins has no cache locking, so a background prewarm download racing a first-use
pin_read()could leave a truncated cache entry. Both sides now take an exclusivefilelockper board cache + pin (the new filelock dependency); lock files are left behind deliberately, since unlinking one a waiter holds would break the exclusion.Also in this PR
source_describe()gets the same dangling-view retry assource_query().cache_hitspan attribute now probes the store path without filesystem side effects, cache-state-mutating tests snapshot/restore that state, and the view tests go throughdata_source_state()(sources are R6 since Makedata_source(),semantic_layer(), andcontext_layer()output R6 #228).