Skip to content

feat(index): CachedJSONIndex with a bounded, singleflighted cache - #2

Closed
jonyoder wants to merge 1 commit into
mainfrom
cachedjson-18647
Closed

feat(index): CachedJSONIndex with a bounded, singleflighted cache#2
jonyoder wants to merge 1 commit into
mainfrom
cachedjson-18647

Conversation

@jonyoder

@jonyoder jonyoder commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

What

RFD 0001 Phase 3.2's go-pyresolver half — Files served from the per-snapshot index JSON, cached by (package, snapshot).

Tracked as rstudio/package-manager#18647 (re-scoped; see below).

RSFIndex is deliberately not here

RSFIndex needs PPM-private code: depsblob.DecodePackage returns map[string]storetypes.PyPIVersionMetadata (carrying Blocked, BlockingRule, Vulns, MetadataMD5Sum), and src/rsf has no go.mod of its own — it is part of a private module that imports this one. Implementing it here would invert the dependency. Moved to rstudio/package-manager#19437.

§6 already implies this split: DBIndex is "backed by the pypi_projects table", which a public module cannot reach either. So §6's list is an inventory of implementations, not a claim about which repo each lives in. Confirmed with Jonathan before writing code.

Metadata always refuses, on purpose

Metadata always returns ErrMetadataUnavailable. Rev 15 moved the dependency fieldset into the resident RSF specifically so resolution works air-gapped; serving requires_dist out of the CDN document this type already holds would quietly reintroduce a per-package network fetch on the resolution hot path. Refusing keeps that regression from being one convenient edit away, and a test asserts it makes zero HTTP requests.

Versions is served, since the same document lists them and it costs nothing extra.

Wire format taken from the real producer

The fixture is an excerpt of PPM's own index_json_v2 document for click, not a hand-written approximation. That is what surfaced the details an invented fixture gets wrong:

  • digests is a map ({"md5": ..., "sha256": ...}), not a flat sha256 field
  • the timestamp is upload_time_iso_8601, RFC 3339 with fractional seconds
  • wheel vs sdist comes from an explicit packagetype (bdist_wheel/sdist), authoritative over the filename
  • yanked_reason is JSON null when absent
  • requires_python is a comma-separated set (">=2.7, !=3.0.*, ..."), asserted to parse

Cache: explicit byte accounting, not ristretto

Not NIH. PPM's own cachehelpers.BoundedCache exists for exactly this reason, documented in its header: ristretto "cannot size a non-[]byte value, so it admits Go objects with cost 0 and they silently escape the byte budget entirely" (#19374). Explicit accounting is the established pattern, and it spares a public library a cache dependency.

Copy-on-return is load-bearing and tested. Singleflight hands the same value to every coalesced waiter, so copying only on a cache hit leaves the entry shared. Verified by removing the copy and watching -race report a genuine DATA RACE plus a deterministic non-concurrent failure — the exact bug class the issue warned about from PPM's snapshot cache (#19291).

Cache soundness is documented: a key must name immutable content. (package, snapshot) qualifies, with one accepted exception — yanked is mutable within a published snapshot per §5.1, which is what #18650 exists to invalidate.

Memory budget: configurable, conservative default

As the issue asked, an explicit decision rather than a constant. §5.1 targets ~300-500MB, but Server.MemoryCacheSize has a 100MB floor on-prem against 4GB on P3M and this cache is additional to it — so defaulting to the RFD figure would roughly quadruple an on-prem memory floor as a side effect of enabling resolution.

Default: 512 entries / 64 MiB, raisable via CachedJSONConfig. Degradation is LRU eviction, so exceeding the budget costs a refetch of the coldest package, not a failure. An entry larger than the whole budget is not cached at all rather than evicting everything and still not fitting.

Correcting the issue: the "~50-concurrent fan-out" has no basis

The issue asks for "~50-concurrent fan-out". I could not find such a bound in PPM, and I checked: PyPI.DownloadRetryLimit is validated to 1–50 but is a retry cap, and PyPI.DownloadConcurrency is deprecated and ignored (config.go:401). Actual concurrency comes from the generic queue ConcurrencyEnforcer (20/10 by default). Looks like a misreading of the retry range, so no fan-out limiter is implemented here — singleflight already collapses duplicate work per key, and bounding total concurrency is the caller's http.Client/transport decision.

Two judgment calls, both tested and commented

  • Malformed per-file metadata is lenient. An unparseable requires_python leaves the constraint unset rather than dropping the file, because dropping a package's only wheel makes it unresolvable for a reason nobody can see. Strict policy belongs to candidate selection, which can see all the files at once.
  • An unparseable version key is skipped, not fatal, so one bad key cannot hide every other version of the package.

Also: a 5xx is never reported as ErrPackageNotFound (one is retryable, the other tells a resolver to give up), and Files falls back to PEP 440-equal comparison when the document's key spelling differs from the request (1.0 vs 1.0.0).

Testing

  • 93.5% statement coverage; -race -count=1 clean; gofmt -l . silent; golangci-lint run ./... at CI's pinned v2.11.2 → 0 issues from the module root.
  • Air-gap injection point is CachedJSONConfig.Client, so a local-disk transport needs no change here.

🤖 Generated with Claude Code

Implements RFD 0001 Phase 3.2's go-pyresolver half: Files served from the
per-snapshot index JSON, cached by (package, snapshot).

RSFIndex is deliberately NOT here. It needs PPM's deps-blob decoder and
store types -- depsblob.DecodePackage returns
map[string]storetypes.PyPIVersionMetadata, carrying Blocked, BlockingRule,
Vulns and MetadataMD5Sum -- and src/rsf has no go.mod of its own, so it is
part of a private module that imports this one. Implementing it here would
invert the dependency. Moved to rstudio/package-manager#19437. RFD Section 6
already implies this split: DBIndex is "backed by the pypi_projects table",
which a public module cannot reach either, so Section 6's list is an
inventory of implementations rather than a claim about which repo each lives
in.

Metadata always returns ErrMetadataUnavailable, on purpose. Rev 15 moved the
dependency fieldset into the resident RSF specifically so resolution works
air-gapped; serving requires_dist out of the CDN document this type already
holds would quietly reintroduce a per-package network fetch on the
resolution path. Refusing keeps that regression from being one convenient
edit away, and a test asserts it makes no HTTP request at all. Versions IS
served, since the same document lists them and it costs nothing extra.

Wire format was taken from the real producer, not approximated: the fixture
is an excerpt of PPM's own index_json_v2 document for click. That is what
surfaced the details a hand-written fixture gets wrong -- digests arrive as
a MAP (not a flat sha256 field), the timestamp field is
upload_time_iso_8601 in RFC 3339 with fractional seconds, wheel-vs-sdist
comes from an explicit packagetype rather than the filename, and
yanked_reason is JSON null when absent.

Cache: a small LRU with an explicit byte budget and singleflight
coalescing, rather than ristretto. Not NIH -- PPM's own
cachehelpers.BoundedCache exists for exactly this reason, documented at
bounded_cache.go: ristretto "cannot size a non-[]byte value, so it admits Go
objects with cost 0 and they silently escape the byte budget entirely"
(#19374). Explicit accounting is the established pattern, and it spares a
public library a cache dependency.

Copy-on-return is load-bearing and tested. Singleflight hands the SAME value
to every coalesced waiter, so copying only on a cache hit leaves the entry
shared; Files copies on the way out. Verified by removing the copy and
watching -race report a genuine DATA RACE plus a deterministic
non-concurrent failure -- the exact bug class that bit PPM's snapshot cache
(#19291).

Memory budget is configurable with a conservative default rather than a
constant, as the issue asked. Section 5.1 targets ~300-500MB, but
Server.MemoryCacheSize has a 100MB floor on-prem against 4GB on P3M and this
cache is ADDITIONAL to it, so defaulting to the RFD figure would roughly
quadruple an on-prem memory floor as a side effect of enabling resolution.
Default is 512 entries / 64 MiB; degradation is LRU eviction, so exceeding
the budget costs a refetch of the coldest package, not a failure.

Two judgment calls, both tested and commented:

  * Malformed per-file metadata is lenient -- an unparseable
    requires_python leaves the constraint unset rather than dropping the
    file, because dropping a package's only wheel makes it unresolvable for
    a reason nobody can see. Strict policy belongs to candidate selection,
    which can see all files at once.
  * An unparseable version key is skipped rather than failing the package,
    so one bad key cannot hide every other version.

Also: a 5xx is never reported as ErrPackageNotFound (one is retryable, the
other tells a resolver to give up), and Files falls back to PEP 440-equal
comparison when the document's key spelling differs from the requested
version ("1.0" vs "1.0.0" are the same version).

Config is named CachedJSONConfig rather than Config, since this package will
gain FilteredIndex and MultiIndex.

Verified: 93.5% statement coverage; -race clean; gofmt silent;
golangci-lint v2.11.2 reports 0 issues from the module root.

Refs rstudio/package-manager#18647

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jonyoder

jonyoder commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

Closing per design discussion with Jonathan.

The problem this solved does not exist under the direction we settled on:

  • PPM will not use this. PPM keeps its own tight, in-process RSF integration (rstudio/package-manager#19437) and already has a production-tested bounded, singleflighted cache over this same document, keyed on the macro id, which is a strictly stronger key than the (package, snapshot) used here since a new sync mints a new macro id and needs no yanked-status caveat.
  • The standalone resolver will not need the per-snapshot JSON at all. Resolution needs version lists and per-version dependencies; post-Rev-15 both are resident in the RSF. The JSON carries only file information, which resolution never consults. If a standalone tool later wants filenames and hashes, upstream PyPI's own index is the right source, not PPM's derived JSON.

So Files() over the CDN JSON is not on the path for either consumer.

Kept from this work: Metadata() returning ErrMetadataUnavailable is a real guardrail against reintroducing a per-package network fetch on the resolution path, and belongs on whichever implementation ships. The wire-format findings are worth keeping as documentation regardless: digests is a map rather than a flat sha256 field, the timestamp is upload_time_iso_8601 in RFC 3339 with fractional seconds, packagetype is authoritative for wheel-vs-sdist over the filename, and yanked_reason is JSON null when absent.

rstudio/package-manager#18647 needs re-scoping again to match. Branch retained for reference.

@jonyoder jonyoder closed this Aug 4, 2026
@jonyoder
jonyoder deleted the cachedjson-18647 branch August 4, 2026 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant