Repository navigation
mcpx: one content-addressed cache mechanism (internal/cache) — #1908 Approach C - #2555
Merged
Merged
Conversation
…Approach C New pkgs/mcpx/internal/cache package implementing Approach C of #1908: - Layout <cache>/<kind>/<key>; keys are SHA-256 of length-prefixed canonical inputs (cache.Key/KeyString). Entries are built in a temp dir and renamed into place, so readers never see a half-written entry. - Leases pin an entry while an exec uses it (lease file inside a directory entry, beside a file entry named for it); eviction never removes a leased entry. Leases older than cache.leaseStaleAge are treated as abandoned by a crashed process. - Eviction by total bytes and age (cache.maxBytes / cache.maxAge, new defaults section), plus an optional per-kind entry count for the small kinds. Any writer evicts its own kind on store — no separate evictor process to race. Runner migration: prunePrograms, pruneWorkDirs and pruneEntries are now thin adapters onto cache.Prune (pattern + keep + limits), keeping their locations and historical count bounds (exec.programs/workDirs/ entries, now limits of the mechanism). pruneWorkDirs also gains the shared byte/age bounds. Run() acquires leases on the workdir, the generated program and the launcher/entry it executes. Surface: `mcpx cache` lists every kind/entry (size, leased, path; --json supported), `mcpx cache prune` runs eviction with the configured bounds, `mcpx cache remove <kind> <key>` deletes one entry and refuses while leased. Local-only, never starts the daemon — the status decision from #1866. docs/parity.md regenerated. Open questions from #1908, answered here: - Who evicts: any writer, on store, for its own kind; the lease is what makes that safe (no daemon evictor). - Pruners: all three move onto the mechanism (above); their built-in count bounds become mechanism limits. - Daemon schema cache (cacheVersion, internal/daemon/registry.go): does NOT join in this PR — it is a single file keyed by config fingerprint (Paths.SchemaCachePath), not a per-kind entry set, and changing its invalidation is #1866 item 3 territory. Recorded as a deliberate non-join, not dropped. - #1913 prepared deps: tenants should store prepared trees via cache.Put under their own kind and let bytes/age eviction bound them; whether to lean on runtime-native caches instead stays #1913's call — the mechanism supports either (a manifest-only entry is just a small entry). Tests: new internal/cache unit tests (key canonicalisation, put/get hit, evict-by-bytes, evict-by-age, leased entry survives eviction, stale lease does not pin, count prune with pattern+keep, remove refuses leased, list). Laptop: go test ./internal/cache ./internal/runner ./internal/cli ./internal/defaults (targeted) pass; binary smoke test of `mcpx cache` list/remove/prune passes. Pre-existing failures on clean main, unchanged by this PR (verified against a pristine origin/main worktree): internal/defaults TestEveryDocumentationLinkResolves, internal/settings TestEveryDocumentHasAChapter. No tenant issue (#1913/#1915/#1921/#1917) is closed by this PR; they integrate under their own issues.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 21 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 962a2757d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This was referenced Oct 30, 2026
Merged
Closed
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
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.
mcpx: one content-addressed cache mechanism (internal/cache) — #1908 Approach C
New pkgs/mcpx/internal/cache package implementing Approach C of #1908:
canonical inputs (cache.Key/KeyString). Entries are built in a temp
dir and renamed into place, so readers never see a half-written entry.
directory entry, beside a file entry named for it); eviction never
removes a leased entry. Leases older than cache.leaseStaleAge are
treated as abandoned by a crashed process.
defaults section), plus an optional per-kind entry count for the
small kinds. Any writer evicts its own kind on store — no separate
evictor process to race.
Runner migration: prunePrograms, pruneWorkDirs and pruneEntries are
now thin adapters onto cache.Prune (pattern + keep + limits), keeping
their locations and historical count bounds (exec.programs/workDirs/
entries, now limits of the mechanism). pruneWorkDirs also gains the
shared byte/age bounds. Run() acquires leases on the workdir, the
generated program and the launcher/entry it executes.
Surface:
mcpx cachelists every kind/entry (size, leased, path;--json supported),
mcpx cache pruneruns eviction with theconfigured bounds,
mcpx cache remove <kind> <key>deletes one entryand refuses while leased. Local-only, never starts the daemon — the
status decision from #1866. docs/parity.md regenerated.
Open questions from #1908, answered here:
what makes that safe (no daemon evictor).
count bounds become mechanism limits.
does NOT join in this PR — it is a single file keyed by config
fingerprint (Paths.SchemaCachePath), not a per-kind entry set, and
changing its invalidation is mcpx: small correctness problems in daemon state and caches #1866 item 3 territory. Recorded as a
deliberate non-join, not dropped.
cache.Put under their own kind and let bytes/age eviction bound
them; whether to lean on runtime-native caches instead stays
mcpx: where a script's packages come from -- npm/deno/bun dependency configuration chain #1913's call — the mechanism supports either (a manifest-only
entry is just a small entry).
Tests: new internal/cache unit tests (key canonicalisation, put/get
hit, evict-by-bytes, evict-by-age, leased entry survives eviction,
stale lease does not pin, count prune with pattern+keep, remove
refuses leased, list). Laptop: go test ./internal/cache ./internal/runner
./internal/cli ./internal/defaults (targeted) pass; binary smoke test
of
mcpx cachelist/remove/prune passes.Pre-existing failures on clean main, unchanged by this PR (verified
against a pristine origin/main worktree): internal/defaults
TestEveryDocumentationLinkResolves, internal/settings
TestEveryDocumentHasAChapter.
No tenant issue (#1913/#1915/#1921/#1917) is closed by this PR; they
integrate under their own issues.
Need help on this PR? Tag
@codesmith-botwith what you need.