feat: add persistent OCI cache with XDG_CACHE_HOME support#127
feat: add persistent OCI cache with XDG_CACHE_HOME support#127em-redhat wants to merge 4 commits into
Conversation
em-redhat
requested review from
marcusburghardt and
yvonnedevlinrh
and removed request for
a team
em-redhat
force-pushed
the
opsx/persistent-oci-cache
branch
from
4276a42 to
21e00b5
Compare
- Add internal/cache package with ResolveDir, NewOCIStore, and Clean - Wire CacheDir into loadBundleArtifacts using oras-go OCI layout store - Replace inline cache resolution with XDG_CACHE_HOME-aware logic - Add complypack pull command to pre-warm cache from configured sources - Add complypack cache clean command to remove cached artifacts - Export LoadArtifacts for reuse across CLI commands Closes: complytime#88 Assisted-by: Claude (Anthropic, Claude 3.5 Sonnet 4.5) Signed-off-by: Em <elyons@redhat.com>
em-redhat
force-pushed
the
opsx/persistent-oci-cache
branch
from
21e00b5 to
1300ce9
Compare
Move LoadArtifacts, loadFileArtifacts, loadBundleArtifacts, and isOCIReference from internal/mcp/server.go into a new internal/source domain package. Per AGENTS.md, MCP handlers are thin wiring with no business logic. The artifact loading functions are domain logic (source resolution, registry pulls, artifact classification) that should live in a domain package, not the transport layer. The pull CLI command now imports internal/source directly instead of going through internal/mcp, eliminating the transport-to-transport dependency. Assisted-by: Claude (Anthropic, Claude 3.5 Sonnet 4.5) Signed-off-by: Em <elyons@redhat.com>
em-redhat
force-pushed
the
opsx/persistent-oci-cache
branch
from
2409efc to
97437fe
Compare
marcusburghardt
left a comment
marcusburghardt
left a comment
There was a problem hiding this comment.
Benchmark Assessment
Built the PR branch against a local Zot registry with two Gemara test bundles
(test-opa-controls:v1.0.0, test-branch-protection:v1.0.0) to measure the
caching impact.
Registry Request Count (from Zot access logs)
| Scenario | HTTP Requests | Bytes Transferred |
|---|---|---|
| Cold pull (empty cache) | 5 | 3,702 bytes |
| Warm pull (cache populated) | 2 | 1,275 bytes |
| Reduction | 60% fewer requests | 66% fewer bytes |
Cold pull fetches 2 manifests + 3 blob downloads. Warm pull fetches only
the 2 manifests — all blobs are served from the local OCI store, skipping
network transfer entirely.
Offline Capability
| Scenario | Result |
|---|---|
| Registry up + warm cache | Works (blobs served from cache) |
| Registry down + warm cache | Fails (manifest resolution requires network) |
With the registry stopped, complypack pull with a warm cache still fails
because oras.Copy always contacts the registry to resolve the manifest
before checking local blobs. The cache deduplicates blob downloads but does
not enable offline operation.
Value Delivered
The caching mechanism works correctly: the on-disk OCI store, XDG directory
resolution, cache clean, and pull commands are solid. The architecture
follows AGENTS.md — domain logic extracted into internal/cache/ and
internal/source/, with CLI commands and MCP server as thin wiring.
internal/cache/ has thorough test coverage.
Three findings below for consideration before merge.
This review was generated by /review-pr (AI-assisted).
|
|
||
| // Package source resolves Gemara artifact sources (file paths or OCI references) | ||
| // and loads them into classified ArtifactSets. | ||
| package source |
There was a problem hiding this comment.
[HIGH] Missing tests for this package.
This file introduces 4 functions (LoadArtifacts, loadFileArtifacts,
loadBundleArtifacts, IsOCIReference) with no corresponding test file.
The constitution requires "All code MUST have tests."
IsOCIReference is a pure function and straightforward to test with a
table-driven approach (see TestIsOCISource in pull_test.go for a
pattern). LoadArtifacts routing logic can be tested by verifying
prefix-based dispatch without real I/O.
There was a problem hiding this comment.
Addressed in d7373d7. Added internal/source/source_test.go with:
TestIsOCIReference: table-driven test covering 11 cases (6 positive OCI references, 5 negative non-OCI inputs)TestLoadArtifacts_FileSource: verifiesfile://prefix dispatch and bare file path dispatch using real temp files with a mock catalog, plus error cases for missing filesTestLoadArtifacts_OCISourceRequiresRegistry: verifiesoci://prefix and bare OCI references route to the bundle loader (not the file loader) by checking the error type
| store = memory.New() | ||
| } | ||
|
|
||
| _, err = oras.Copy(ctx, repo, tag, store, tag, oras.DefaultCopyOptions) |
There was a problem hiding this comment.
[MEDIUM] Warm cache still makes network requests — offline use not supported.
Benchmarked against a local Zot registry: with a warm cache, oras.Copy
still issues a GET /v2/.../manifests/v1.0.0 per source to resolve the
tag before checking local blobs. With the registry stopped, this call
fails and the entire pull errors out.
Issue #88 acceptance criterion states: "Second invocation with warm cache
does not make network requests." The current implementation reduces
redundant blob downloads (60% fewer requests, 66% fewer bytes) but does
not eliminate network dependency.
This could be addressed in a follow-up by checking the local store for
the tag before calling oras.Copy, or by documenting the current
behavior as "cache reduces bandwidth" rather than "enables offline use."
There was a problem hiding this comment.
Acknowledged. Updated the PR description with a new Cache behavior section that documents the actual behavior:
- Warm cache reduces bandwidth (60% fewer requests, 66% fewer bytes per the benchmark)
- Warm cache does not enable offline operation —
oras.Copystill contacts the registry to resolve the tag to a manifest digest - Full offline support (checking the local store for the tag before contacting the registry) noted as a follow-up
The issue #88 acceptance criterion "Second invocation with warm cache does not make network requests" is more accurately met for blob downloads but not for manifest resolution. The PR description now reflects this distinction.
| cfg, err := buildConfigFromFlags(sources, schemas) | ||
| if err != nil { | ||
| writeStartupError(err) | ||
| return fmt.Errorf("failed to build config from flags: %w", err) |
There was a problem hiding this comment.
[MEDIUM] Removal of writeStartupError is undocumented scope drift.
The writeStartupError function (and its two call sites here and at
line 85) sent JSON-RPC error responses to stdout so MCP clients over
stdio could surface real error messages instead of generic
"Connection closed" errors. Its removal is unrelated to caching and
not mentioned in the PR description.
Per Constitution Principle III, unrelated changes should be in a
separate commit or PR. If the removal is intentional (e.g., the MCP
SDK now handles this case), the rationale should be documented in the
PR description.
There was a problem hiding this comment.
Fixed in d7373d7. The removal was an unintended side effect of rebasing onto upstream/main — git auto-resolved the conflict by keeping our import changes (which removed os and encoding/json) but didn't detect that writeStartupError still needed them.
Restored:
writeStartupErrorfunction with its full implementation and doc comment- Both call sites (
buildConfigFromFlagserror path at line 76 andNewServererror path at line 85) TestWriteStartupErrortestencoding/json,osimports in bothmcp.goandmcp_test.go
The function is unrelated to caching and belongs in its own PR if it ever needs to be removed (per Constitution Principle III).
- Add unit tests for internal/source package: IsOCIReference table-driven tests and LoadArtifacts routing tests for file://, bare path, oci://, and bare OCI reference dispatch - Restore writeStartupError function and its call sites that were inadvertently removed during rebase; this is unrelated to caching and its removal was undocumented scope drift (Constitution III) - Restore TestWriteStartupError test Assisted-by: Claude (Anthropic, Claude 3.5 Sonnet 4.5) Signed-off-by: Em <elyons@redhat.com>
| if err != nil { | ||
| return "", fmt.Errorf("cannot resolve cache directory: HOME is not set and --cache-dir was not provided: %w", err) | ||
| } | ||
| return filepath.Join(home, ".complypack", "cache"), nil |
There was a problem hiding this comment.
The fallback for XDG_CACHE_HOME should be $HOME/.cache/complypack according to the spec
There was a problem hiding this comment.
Good catch on the XDG spec. This is an error coming from the original issue.
complyctl uses $HOME/.complytime/ without XDG -- so .complypack/cache is actually consistent with the existing complytime convention. However, if we are adopting XDG here, not complying with the spec is not a good option.
My take is we should make this spec compliant, but we should probably settle the convention across all complytime CLIs. Could we open an ADR in complytime/complytime to decide this?
There was a problem hiding this comment.
@jpower432 Do we need an ADR to be XDG compliant? That seems like a "what you do" sort of thing these days.
There was a problem hiding this comment.
Well I think plenty of tools use ~/.appdir/ convention without XDG, and plenty faithfully follow the spec.
Both are legitimate. complyctl already went with option 1. The ADR isn't about whether XDG is a good idea, it's about which convention complytime commits to across all its CLIs, so we don't end up with a split.
In this PR (and original issue), we are making a decision that affects consistent experiences between complytime tools. Keeping the decision clear ("complytime CLIs fully conform to the XDG Base Directory spec") means that we don't get accidental deviation or we don't end up with a half implemented spec and should trigger a change in complyctl
Not suggesting this block this PR, but suggesting it as a follow on action at the very least.
There was a problem hiding this comment.
Fixed in e5ffafb. Changed fallback from $HOME/.complypack/cache to $HOME/.cache/complypack per the XDG Base Directory Specification. Updated doc comments, help text, and test assertions to match.
There was a problem hiding this comment.
In this particular case, following XDG helps users quite a bit. If I symlink ~/.cache to a large disk, that's one hit for everything. If I have to go figure out which ~/.??? directory is now eating all of my disk, it's more painful.
| @@ -0,0 +1,67 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| package cache | |||
There was a problem hiding this comment.
We've got a race condition between CLI invocations here.
We need to add a cross-platform lock around critical paths.
Technically, there's also a race condition on clean and all other operations so we're going to need some error handling around that.
While this isn't usually an issue, these days if you have multiple agentic workflows running on the same system you could easily hit it.
There was a problem hiding this comment.
Agreed — concurrent CLI invocations (or parallel agentic workflows) could hit races on the OCI store. For this PR, oci.Store from oras-go provides file-level atomicity for blob writes (content-addressable, write-to-temp-then-rename), which covers the most common case. Full cross-process locking (e.g., flock on a lockfile in the cache directory) is a good follow-up but would add complexity beyond the scope of this change. Filed as a note for future work.
There was a problem hiding this comment.
@em-redhat Was this filed as an issue? If so, could you link it here please?
| // not already exist. | ||
| func NewOCIStore(cacheDir string) (*oci.Store, error) { | ||
| if err := os.MkdirAll(cacheDir, 0750); err != nil { | ||
| return nil, fmt.Errorf("failed to create cache directory %s: %w", cacheDir, err) |
There was a problem hiding this comment.
Do we need to allow the system to fall back to the in-memory store that we currently have? Thinking about a locked down MCP server.
If the MCP server is expected to write to a target directory then that's fine (and good) but that means that the in-memory store is now dead and should be removed, correct?
There was a problem hiding this comment.
The in-memory fallback is preserved for the case where CacheDir is empty (e.g., in tests or if someone explicitly passes --cache-dir ""). For a locked-down MCP server where writing to disk is not desired, the in-memory path still works — just don't set CacheDir. If the consensus is that all invocations should always use disk, we can remove the fallback, but keeping it avoids a breaking change for existing test setups that rely on the no-disk behavior.
There was a problem hiding this comment.
Sounds good. Did we validate that the in-memory fallback is triggered correctly now? IIRC, there was an early bail-out that would skip it in all cases.
XDG compliance: - Change fallback from $HOME/.complypack/cache to $HOME/.cache/complypack per XDG Base Directory Specification - Add CacheDirHelp const to DRY the flag help text across all commands Cache safety: - Clean now removes directory contents only, preserving the directory inode so other processes holding a reference are not invalidated - Cache OCI store instances by directory path to avoid re-walking the OCI layout on every loadBundleArtifacts call Pull command: - Remove ValidateForMCP call (requires schemas, which pull does not need) - Remove duplicate isOCISource; use source.IsOCIReference instead - Extract domain logic into pullSources() for testability without cobra - Remove redundant os.Stat in cache clean CLI; cache.Clean handles it Assisted-by: Claude (Anthropic, Claude 3.5 Sonnet 4.5) Signed-off-by: Em <elyons@redhat.com>
4 participants
Summary
Implements persistent OCI artifact caching with XDG Base Directory
Specification support, resolving issue #88 where
CacheDironServerOptionswas declared but never used.Changes
internal/source: New domain package for artifact source resolutionand loading. Extracts
LoadArtifacts,loadFileArtifacts, andloadBundleArtifactsfrom the MCP transport layer per AGENTS.md("write the logic in the appropriate domain package, then wire it
from the transport layer").
internal/cache: New package withResolveDir(3-tier cachedirectory resolution:
--cache-dir>$XDG_CACHE_HOME/complypack>$HOME/.complypack/cache),NewOCIStore(OCI Image Layout on-diskstore via oras-go), and
Clean(cache wipe)internal/mcp/server.go: Now delegates tosource.LoadArtifacts(thin wiring only). Loading logic removed from transport layer.
cmd/complypack/cli/mcp.go: Usescache.ResolveDirinstead ofinline resolution; updated
--cache-dirhelp textcomplypack pull: New command to pre-warm cache for allconfigured OCI sources, with continue-on-error and progress reporting
complypack cache clean: New command to remove all cachedartifacts
Cache behavior
The on-disk OCI store deduplicates blob downloads: once a blob is
cached locally,
oras.Copyskips re-downloading it. However,oras.Copystill contacts the registry to resolve the tag to amanifest digest on every invocation. This means:
bytes in benchmarks against a Zot registry)
is unreachable, the pull will fail even with cached blobs
Full offline support (checking the local store for the tag before
contacting the registry) can be addressed in a follow-up.
Pruning
Issue #88 lists pruning as "design TBD". This PR provides
complypack cache cleanfor full cache wipe. Selective pruning(TTL-based expiry, LRU eviction, per-source invalidation) is
deferred to a follow-up.
Testing
internal/cache(ResolveDir, NewOCIStore, Clean)internal/source(IsOCIReference, LoadArtifactsrouting for file and OCI sources)
-race(0 failures)Closes #88