Skip to content

feat(upstreams): route by priority and package-name globs, not by alphabet - #79

Merged
BirknerAlex merged 1 commit into
mainfrom
feat/upstream-priority-and-package-patterns
Sep 9, 2026
Merged

BirknerAlex merged 1 commit into
mainfrom
feat/upstream-priority-and-package-patterns

Conversation

@BirknerAlex

@BirknerAlex BirknerAlex commented Sep 9, 2026

Copy link
Copy Markdown
Owner

Two upstreams of one format were tried in name order, and every one of them was asked about every package. That is fine for interchangeable mirrors and wrong for anything else — a vendor registry holding one scope next to a public registry holding the rest.

It fails quietly rather than loudly. A vendor registry typically answers for public names too, by proxying or 302-redirecting to the public registry, and reqwest follows redirects. So the first upstream by name wins every package, the packages are cached through its entry, and the whole repo ends up attributed to it in silo list.

What this looked like in production

A silo with fontawesome and npmjs configured on one channel:

silo_upstream_packages_synced{name="fontawesome"} 69393
silo_upstream_packages_synced{name="npmjs"}        7098

118 of 124 stored npm packages — @babel/core, @eslint/eslintrc, @nuxt/image, vue — recorded as upstream:fontawesome. fontawesome sorts before npmjs, and https://npm.fontawesome.com/<public-package> answers 302 → registry.npmjs.com, so it won every name and silo cached npmjs's packages through the vendor entry. The operator's no_cache setting on npmjs was bypassed as a side effect, because everything arrived through the cache-mode entry instead.

(The vendor credential is not leaked doing this — reqwest strips Authorization on a cross-host redirect.)

Change

Two columns on upstreams, both defaulting to today's behaviour so an existing deployment is unaffected until someone opts in:

  • priority — tried highest first, ties broken by name. The order becomes explicit instead of an accident of the alphabet, and reordering no longer means renaming.
  • package_patterns — globs (*, ?) the package name must match for an upstream to be consulted at all. Empty means no restriction. Repeatable, so one upstream can hold several scopes.
silo repo add-upstream vawld --channel main --name fontawesome --format npm \
  --url https://npm.fontawesome.com --cache --bearer-token "$FA_TOKEN" \
  --priority 10 --package-pattern '@fortawesome/*'

silo repo list-upstreams now shows both, in the order upstreams are actually tried:

NAME         FORMAT  BASE_URL                     CACHE     PRIO  PACKAGES         AUTH   STATUS
fontawesome  npm     https://npm.fontawesome.com  cache     10    @fortawesome/*   true   ok
npmjs        npm     https://registry.npmjs.org   no_cache  0     *                false  ok

The pattern filter is applied everywhere the package name is known: the artifact miss path, npm's lazy per-name sync, and the index merge — an upstream must not advertise a name it is not allowed to serve, or a client would ask for it and be turned away after the fact. update_upstream takes its settings as a struct rather than a tenth positional argument.

The glob matcher is hand-rolled (~20 lines, two metacharacters, no new dependency). * spans / so @acme/* covers a whole scope; the match is against the whole name, not a prefix.

Tests

  • Six unit tests on the matcher and upstream_serves, including the case this exists for: @fortawesome/* must not match @babel/core.
  • upstreams_are_listed_highest_priority_first_then_by_name and routing_settings_round_trip_and_default_to_unrestricted at the DB layer.
  • an_upstream_scoped_to_a_pattern_is_not_consulted_for_other_names builds the production shape — a vendor registry that also answers for public names, given the higher priority and the earlier name, so only the pattern keeps it away — and asserts each package's origin_upstream_id plus that the vendor registry was never asked. Without the filter it fails with lodash was served by the wrong upstream: left: "vendor", right: "public".

Verified against a running server with a registry that 302s unknown names to npmjs: without a pattern, left-pad and ms are stored as upstream:vendor; with --package-pattern '@vendor/*' they are upstream:npmjs and the vendor registry is not asked about them at all.

cargo test --workspace (484 tests), cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --check and ci/check-chart.py are clean.

Not included

The underlying reason a redirect can silently re-route a request — silo follows cross-host redirects on metadata fetches and still attributes the result to the upstream it started from — is untouched. A pattern now prevents it for anyone who sets one; without one, the misattribution is still possible.

The wiki has no page covering pull-through at all (Introduction.md still lists "mirroring or upstream proxying" as out of scope), so there is nowhere to put these two flags. Worth a page of its own rather than a line squeezed in here.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 29 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 8ea1e510-249e-4842-b670-9912b88c78f8

📥 Commits

Reviewing files that changed from the base of the PR and between df9d16b and 3b4b02d.

📒 Files selected for processing (11)
  • crates/silo-cli/src/main.rs
  • crates/silo-core/src/pull_through.rs
  • crates/silo-core/src/repo.rs
  • crates/silo-core/src/upstream_sync.rs
  • crates/silo-db/migrations/0006_upstream_routing.sql
  • crates/silo-db/src/upstreams.rs
  • crates/silo-server/src/admin.rs
  • crates/silo-server/src/http.rs
  • crates/silo-server/tests/publish_flow.rs
  • crates/silo-server/tests/upstream_pull_through.rs
  • proto/silo/v1/admin.proto

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@BirknerAlex
BirknerAlex force-pushed the feat/upstream-priority-and-package-patterns branch from a2a4271 to ecbbcbc Compare September 9, 2026 15:29
…habet

Two upstreams of one format were tried in name order, and every one of
them was asked about every package. That is fine for interchangeable
mirrors and wrong for anything else -- a vendor registry holding one
scope next to a public registry holding the rest.

It fails quietly rather than loudly. A vendor registry typically answers
for public names too, by proxying or 302-redirecting to the public
registry, and reqwest follows redirects: so the first upstream by name
wins every package, the packages are cached through *its* entry, and the
whole repo ends up attributed to it in `silo list`. A production instance
with `fontawesome` and `npmjs` configured had 69,393 synced rows against
npmjs's 7,098, and 118 of 124 stored packages -- @babel/core, @eslint/*,
@nuxt/image -- recorded as coming from the vendor registry, which had in
fact redirected every one of them to npmjs.

Two columns, both defaulting to today's behaviour:

- `priority`, tried highest first, ties broken by name, so the order is
  explicit instead of an accident of the alphabet and reordering does not
  mean renaming.
- `package_patterns`, globs (`*`, `?`) the package name must match for an
  upstream to be consulted at all. Empty means no restriction, so nothing
  changes until an operator opts in. Repeatable, so one upstream can hold
  several scopes.

The filter is applied everywhere the package name is known: the artifact
miss path, npm's lazy per-name sync, and the index merge -- an upstream
must not advertise a name it is not allowed to serve, or a client would
ask for it and be turned away after the fact.

`update_upstream` takes its settings as a struct rather than a tenth
positional argument.

Verified against a running server with a registry that 302s unknown names
to npmjs, exactly as the vendor one does: without a pattern, `left-pad`
and `ms` are stored as `upstream:vendor`; with `--package-pattern
'@vendor/*'` they are `upstream:npmjs` and the vendor registry is not
asked about them at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@BirknerAlex
BirknerAlex force-pushed the feat/upstream-priority-and-package-patterns branch from ecbbcbc to 3b4b02d Compare September 9, 2026 15:59
@BirknerAlex
BirknerAlex merged commit 7090b26 into main Sep 9, 2026
6 checks passed
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