Refactor FsRepository's renders by jrray · Pull Request #1159 · spkenv/spk

jrray · 2024-12-07T02:08:30Z

The goal is to make it so it is possible to open repo without creating the renders directory for the current user, in situations where that renders directory will not be used. Examples include when using fuse or when running spfs clean.

This introduces different flavors of a "render store" besides RenderStore that either represent a render store that hasn't been created yet (but can be) or the lack of a render store. Operations that don't require a render store can access the local repository without the renders directory being created.

~~WIP: The test suite passes¹ with this code but it is unfinished.~~

On my machine... A limitation of the test suite is that it uses the installed spfs binaries instead of the spfs code in the current branch. ↩

jrray · 2024-12-10T02:25:25Z

Tests are now passing legitimately (at least locally, with an spfs installed from this code). What's left besides general cleanup is creating some new tests that specifically exercise the desired outcome: not creating the renders directory if using fuse. I'm not sure if that is true yet with these changes but the framework for making it possible should be there.

jrray · 2024-12-11T02:28:36Z

Making progress but the behavior of spfs clean is not right yet (it currently skipping cleaning renders entirely). The existing tests via the fixture create a repo with RenderStore (pre-created renders) but spfs renders uses MaybeRenderStore, then the clean code treats this flavor as not having renders.

I'm trying to get it so spfs clean does clean any existing renders that are eligible without creating any new renders directories.

codecov · 2026-03-06T01:40:09Z

Codecov Report

❌ Patch coverage is 70.72491% with 315 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/spfs/src/storage/fs/repository.rs	70.90%	119 Missing ⚠️
crates/spfs/src/storage/fallback/repository.rs	9.52%	57 Missing ⚠️
crates/spfs/src/storage/fs/renderer.rs	20.00%	36 Missing ⚠️
crates/spfs/src/resolve.rs	31.42%	24 Missing ⚠️
crates/spfs/src/storage/fs/repository_test.rs	93.21%	19 Missing ⚠️
crates/spfs/src/storage/handle.rs	61.70%	18 Missing ⚠️
crates/spfs/src/config.rs	68.42%	12 Missing ⚠️
crates/spfs-vfs/src/fuse.rs	0.00%	8 Missing ⚠️
crates/spfs-cli/cmd-render/src/cmd_render.rs	0.00%	4 Missing ⚠️
crates/spfs/src/runtime/storage.rs	42.85%	4 Missing ⚠️
... and 9 more

📢 Thoughts on this report? Let us know!

Copilot

Pull request overview

Refactors SPFS filesystem repositories to make render-store creation optional (or deferred) so the local repo can be opened without creating renders/ for the current user when renders aren’t needed (e.g., FUSE paths, spfs clean).

Changes:

Introduces typed “render store flavors” (RenderStore, MaybeRenderStore, NoRenderStore) and threads them through OpenFsRepository/MaybeOpenFsRepository, FallbackProxy, and RepositoryHandle.
Updates call sites across SPK/SPFS crates to select an appropriate repo flavor based on whether renders are required.
Adds an integration test asserting that FUSE-backed operations do not create the renders/ directory.

Reviewed changes

Copilot reviewed 60 out of 61 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
cspell.json	Adds `mktemp` to dictionary for new shell test.
crates/spk/src/lib.rs	Normalizes `pub use` re-export style.
crates/spk-storage/src/storage/spfs_test.rs	Updates tests to explicitly create repos with `RenderStore`.
crates/spk-storage/src/storage/spfs.rs	Opens local repo as `MaybeRenderStore` to avoid eager renders creation.
crates/spk-storage/src/fixtures.rs	Updates fixtures to create repos with `RenderStore`.
crates/spk-solve/src/lib.rs	Normalizes re-exports; exposes serde/spfs and solve subcrates.
crates/spk-solve/crates/macros/src/lib.rs	Normalizes re-exports used by macros.
crates/spk-launcher/src/main.rs	Uses `OpenFsRepository<NoRenderStore>` in launcher path.
crates/spk-cli/group4/src/cmd_view.rs	Minor import consolidation.
crates/spk-cli/group3/src/cmd_import_test.rs	Adjusts expectations: repo layout no longer requires `renders/`.
crates/spk-cli/group3/src/cmd_export_test.rs	Adjusts expectations: repo layout no longer requires `renders/`.
crates/spk-cli/group2/src/cmd_ls.rs	Import reshuffle; explicit `spk_config` / `spk_storage` usage.
crates/spk-cli/group1/src/cmd_completion.rs	Import consolidation for `clap_complete`.
crates/spk-cli/common/src/flags.rs	Import reshuffle; explicit `solve`/`storage` aliases.
crates/spk-cli/cmd-render/src/cmd_render.rs	Opens local repo with `NoRenderStore` for render command path that shouldn’t force renders.
crates/spk-build/src/build/binary_test.rs	Uses `get_local_repository::<RenderStore>()` in build tests needing renders.
crates/spfs/tests/integration/unprivileged/test_fuse_does_not_create_renders.sh	New integration test asserting FUSE does not create `renders/`.
crates/spfs/src/sync_test.rs	Uses `NoRenderStore` for sync tests that don’t need renders.
crates/spfs/src/storage/tar/repository.rs	Tar repository uses `MaybeOpenFsRepository<NoRenderStore>` internally.
crates/spfs/src/storage/tag_test.rs	Updates test repo creation to `RenderStore`.
crates/spfs/src/storage/repository_test.rs	Updates fs repo creation to `RenderStore` and handle variant matches.
crates/spfs/src/storage/repository.rs	Splits repository traits into payload vs render-store capabilities (`LocalPayloads`, `TryRenderStore`, etc.).
crates/spfs/src/storage/proxy/repository_test.rs	Updates proxy repo tests to create repos with `RenderStore`.
crates/spfs/src/storage/mod.rs	Re-exports new repository/render-store traits and defaults.
crates/spfs/src/storage/handle.rs	Expands `RepositoryHandle` variants to encode render-store capability.
crates/spfs/src/storage/fs/tag.rs	Generalizes tag storage impls over render-store flavor type parameter.
crates/spfs/src/storage/fs/repository.rs	Core refactor: render store flavors, creation policies, and generic fs repo types.
crates/spfs/src/storage/fs/renderer_win.rs	Renderer now depends on `LocalPayloads + TryRenderStore` instead of `LocalRepository`.
crates/spfs/src/storage/fs/renderer_unix.rs	Renderer now uses `try_render_store()`; avoids hard dependency on renders.
crates/spfs/src/storage/fs/renderer_test.rs	Updates renderer tests to use `RenderStore` variant.
crates/spfs/src/storage/fs/renderer.rs	Splits rendering APIs based on whether repo has a guaranteed render store.
crates/spfs/src/storage/fs/payloads.rs	Generalizes payload storage impls over render-store flavor.
crates/spfs/src/storage/fs/mod.rs	Re-exports new fs storage types (render store flavors, policies).
crates/spfs/src/storage/fs/manifest_render_path.rs	Adds `ManifestRenderPath` impl for `Arc<T>`.
crates/spfs/src/storage/fs/hash_store.rs	Adds `Debug` derive to `FsHashStore`.
crates/spfs/src/storage/fs/database.rs	Generalizes database impls over render-store flavor.
crates/spfs/src/storage/fallback/repository_test.rs	Updates fallback proxy tests to use `FallbackProxy<RenderStore>`.
crates/spfs/src/storage/fallback/repository.rs	Makes `FallbackProxy` generic over render-store flavor and updates conversions/traits.
crates/spfs/src/storage/error.rs	Adds new open-repo errors for render-store unavailability/policy mismatch.
crates/spfs/src/status_unix.rs	Makes overlay render path explicit with `RenderStore` generic selection.
crates/spfs/src/runtime/storage_test.rs	Updates runtime storage tests to create fs repos with `RenderStore`.
crates/spfs/src/runtime/storage.rs	Updates durable-path lookup for new `RepositoryHandle` variants.
crates/spfs/src/resolve_test.rs	Updates tests to open local repo with `RenderStore` where required.
crates/spfs/src/resolve.rs	Uses `NoRenderStore` for manifest computation; makes overlay render path generic over render-store type.
crates/spfs/src/fixtures.rs	Adds new tmprepo kinds for render-store flavors and updates fixtures accordingly.
crates/spfs/src/config_test.rs	Creates remote fs repo with `NoRenderStore`.
crates/spfs/src/config.rs	Makes local repo getters generic over render-store flavor; forces remotes to `NoRenderStore`.
crates/spfs/src/commit_test.rs	Updates repo creation calls to specify `RenderStore`.
crates/spfs/src/clean_test.rs	Expands tests across repo flavors; refactors render-clean test helper.
crates/spfs/src/clean.rs	Updates cleaning logic to operate with optional/deferred render store and avoid creating renders.
crates/spfs/src/bootstrap_test.rs	Updates repo creation calls to specify `RenderStore`.
crates/spfs/benches/spfs_bench.rs	Uses `NoRenderStore` for benchmark repo creation.
crates/spfs/Cargo.toml	Adds `variantly` dependency (workspace) for render-store policy enum helpers.
crates/spfs-vfs/src/winfsp/mount.rs	Updates VFS read path to handle new `RepositoryHandle` FS variants.
crates/spfs-vfs/src/fuse.rs	Updates FUSE read path to handle new `RepositoryHandle` FS variants.
crates/spfs-cli/main/src/cmd_search.rs	Uses `NoRenderStore` when searching tags to avoid renders creation.
crates/spfs-cli/main/src/cmd_init.rs	Initializes repo as `NoRenderStore` (no renders created at init).
crates/spfs-cli/common/src/lib.rs	Normalizes private re-export style.
crates/spfs-cli/common/src/args.rs	Uses `LocalPayloads` instead of `LocalRepository` for renderer construction.
crates/spfs-cli/cmd-render/src/cmd_render.rs	Splits render-to-dir vs render-to-repo flows using `MaybeRenderStore` vs `RenderStore`.
Cargo.lock	Records new dependency usage (`variantly`).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Expand the clean tests to test the multiple flavors of FS repo. Signed-off-by: J Robert Ray <jrray@jrray.org>

Signed-off-by: J Robert Ray <jrray@jrray.org>

Change from_config to return a RepositoryHandle instead of a specific repository type. The conflict stems from the config containing options that would influence what specific repository type is needed to respect the options. Therefore, callers using from_config shouldn't be expected to know which specific repository type is needed, and this keeps the logic of choosing the appropriate arm of RepositoryHandle internal to the individual repository implementations. Signed-off-by: J Robert Ray <jrray@jrray.org>

Signed-off-by: J Robert Ray <jrray@jrray.org>

Copilot

Pull request overview

Copilot reviewed 66 out of 67 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

renders_for_all_users now gracefully skips per-user render stores that fail with PathNotInitialized or RenderStorageUnavailable instead of aborting the entire operation. This prevents commands like spfs clean from failing due to a single bad or missing per-user proxy directory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: J Robert Ray <jrray@jrray.org>

Copilot

Pull request overview

Copilot reviewed 66 out of 67 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Some RS types like MaybeRenderStore defer on-disk validation in render_store_for_user(), so it always succeeds even when the proxy directory is missing. Add a direct filesystem check for the proxy directory before calling render_store_for_user to ensure users with missing or invalid proxy directories are skipped regardless of the RS type. Also adds a test using MaybeRenderStore to verify the deferred validation path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: J Robert Ray <jrray@jrray.org>

…er blob When try_render_store() returns Err, the else branch now returns a proper Error::NoRenderStorage with the repository address, making the error matchable and consistent with how this case is handled elsewhere (e.g. in clean.rs). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: J Robert Ray <jrray@jrray.org>

Copilot

Pull request overview

Copilot reviewed 66 out of 67 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jrray added the pr-chain This PR doesn't target the main branch, don't merge! label Dec 7, 2024

jrray self-assigned this Dec 7, 2024

jrray marked this pull request as draft December 7, 2024 02:08

jrray force-pushed the repo-renders-type-state branch from 8d8f699 to edf84c4 Compare December 10, 2024 02:22

jrray force-pushed the no-hard-links-for-dir-renders branch from 7ec8e8b to 51a8e45 Compare December 10, 2024 18:58

jrray force-pushed the repo-renders-type-state branch 5 times, most recently from 631cc72 to a367c0f Compare December 11, 2024 00:59

jrray mentioned this pull request Dec 11, 2024

spfs clean fails if disk is full #923

Open

jrray linked an issue Dec 11, 2024 that may be closed by this pull request

spfs clean fails if disk is full #923

Open

This was referenced Jan 7, 2025

Make environment variables to preserve configurable #1148

Merged

Draft: explore type state for FSRepository #924

Closed

jrray force-pushed the no-hard-links-for-dir-renders branch from 51a8e45 to 470ac8b Compare March 29, 2025 02:27

jrray force-pushed the repo-renders-type-state branch from 051f03e to 8140bae Compare March 29, 2025 02:28

jrray force-pushed the repo-renders-type-state branch from 8140bae to e3cf272 Compare May 23, 2025 23:14

jrray force-pushed the no-hard-links-for-dir-renders branch from 470ac8b to 60add1c Compare May 23, 2025 23:14

Base automatically changed from no-hard-links-for-dir-renders to main May 23, 2025 23:28

jrray removed the pr-chain This PR doesn't target the main branch, don't merge! label May 23, 2025

jrray force-pushed the repo-renders-type-state branch 2 times, most recently from acfea7f to f866c95 Compare March 6, 2026 01:26

jrray force-pushed the repo-renders-type-state branch from f866c95 to e665f18 Compare March 6, 2026 01:45

jrray marked this pull request as ready for review March 6, 2026 01:58

jrray requested a review from Copilot March 6, 2026 01:58

Copilot started reviewing on behalf of jrray March 6, 2026 01:58 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

Comment thread crates/spfs/src/storage/fs/repository.rs

Comment thread crates/spfs/src/storage/fs/repository.rs

Comment thread crates/spfs/src/clean_test.rs Outdated

Comment thread crates/spfs/src/storage/fallback/repository.rs Outdated

jrray added 13 commits March 6, 2026 16:50

Fix clean silently ignoring MaybeRenders type

75b6b30

Expand the clean tests to test the multiple flavors of FS repo. Signed-off-by: J Robert Ray <jrray@jrray.org>

Make clean not create renders

dd214c5

Signed-off-by: J Robert Ray <jrray@jrray.org>

Stop shadowing RenderStore type name in generic parameter

49c2d81

Signed-off-by: J Robert Ray <jrray@jrray.org>

Implement FallbackProxy::render_store_for_user

b05ad01

Signed-off-by: J Robert Ray <jrray@jrray.org>

spfs: honor create_renders policy and avoid maybe-render poisoning

b4aca4b

Signed-off-by: J Robert Ray <jrray@jrray.org>

spfs: limit clean_manifest_renders test to render-capable repos

22b66b6

Signed-off-by: J Robert Ray <jrray@jrray.org>

spfs: test render store creation policy behavior

8539321

Signed-off-by: J Robert Ray <jrray@jrray.org>

spfs: test maybe-render to render conversion paths

845eef9

Signed-off-by: J Robert Ray <jrray@jrray.org>

spfs: test fallback proxy render-store variant behavior

3b9f821

Signed-off-by: J Robert Ray <jrray@jrray.org>

spfs: test resolve stack across repository handle variants

b467146

Signed-off-by: J Robert Ray <jrray@jrray.org>

spfs: test renderer behavior without render store

a2f84ac

Signed-off-by: J Robert Ray <jrray@jrray.org>

spfs: pass username segment in renders_for_all_users and test it

693ad96

Signed-off-by: J Robert Ray <jrray@jrray.org>

jrray force-pushed the repo-renders-type-state branch from 99eb63d to 693ad96 Compare March 7, 2026 01:16

spfs: only create render proxy dir when metadata is missing

9674335

Signed-off-by: J Robert Ray <jrray@jrray.org>

jrray requested a review from Copilot March 7, 2026 02:00

Copilot started reviewing on behalf of jrray March 7, 2026 02:00 View session

Copilot AI reviewed Mar 7, 2026

View reviewed changes

Comment thread crates/spfs/src/config.rs

Comment thread crates/spfs/src/storage/fs/repository.rs Outdated

jrray added the AI Code authored with AI assistance. label Mar 7, 2026

jrray requested a review from Copilot March 7, 2026 02:32

Copilot started reviewing on behalf of jrray March 7, 2026 02:33 View session

Copilot AI reviewed Mar 7, 2026

View reviewed changes

Comment thread crates/spfs/src/storage/fs/repository.rs

Comment thread crates/spfs/src/storage/fs/renderer_unix.rs

jrray and others added 2 commits March 6, 2026 18:54

jrray force-pushed the repo-renders-type-state branch from cc6345c to 66d457d Compare March 7, 2026 04:10

jrray requested a review from Copilot March 7, 2026 04:24

Copilot started reviewing on behalf of jrray March 7, 2026 04:25 View session

Copilot AI reviewed Mar 7, 2026

View reviewed changes

Comment thread crates/spfs/src/clean.rs

jrray requested a review from rydrman March 7, 2026 04:37

Navigation Menu

Uh oh!

Refactor FsRepository's renders - #1159

Refactor FsRepository's renders#1159
jrray wants to merge 19 commits into
mainfrom
repo-renders-type-state

jrray commented Dec 7, 2024 •

edited

Loading

Uh oh!

jrray commented Dec 10, 2024

Uh oh!

jrray commented Dec 11, 2024

Uh oh!

codecov Bot commented Mar 6, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jrray commented Dec 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

jrray commented Dec 10, 2024

Uh oh!

jrray commented Dec 11, 2024

Uh oh!

codecov Bot commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jrray commented Dec 7, 2024 •

edited

Loading

codecov Bot commented Mar 6, 2026 •

edited

Loading