fix(registry): write the packument cache with serde_json, not sonic-rs - #668
Conversation
A `serde_json::Value` may only be handed to serde_json's own serializer.
With `serde_json/arbitrary_precision` anywhere in the feature graph,
`Number::serialize` emits `serialize_struct("$serde_json::private::Number", 1)`
and relies on the serializer to intercept that token. serde_json does;
sonic-rs writes it out literally, turning every number in the document into
`{"$serde_json::private::Number":"27353961"}` on disk.
Cargo unifies features per build, so one dependency turns this on for the
whole graph — `nub compile` pulls `rolldown_common`, which requires it. The
packument cache is shared by every build on the machine, so such a build
poisons it for other binaries, and the damage is self-perpetuating: a
poisoned entry reads back as a plain map and ETag revalidation rewrites it
verbatim, so it never heals. The visible symptom is
`invalid type: map, expected u64` on the next typed read.
Route both cache writers through one `to_cache_bytes` seam on
`serde_json::to_vec`. The read path stays on sonic-rs — deserializing into a
`Value` has no equivalent hazard, and that is where the throughput matters.
Beyond `unpackedSize` and `fileCount`, `Packument` reaches
`serde_json::Value` through `dist.attestations.provenance`,
`_npmUser.trustedPublisher` and `approver`. The latter two are trust-policy
evidence, so a corrupted one degrades a security decision without failing a
parse.
The regression test goes through `RawValue` rather than the number token:
serde_json both emits and intercepts the number token under
`#[cfg(feature = "arbitrary_precision")]`, so in a default-feature build
neither half is compiled and no assertion can tell the serializers apart.
`raw_value` is on by default and is the testable instance of the same
invariant. It carries a failing control asserting sonic-rs leaks the token.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ The fix is correct — one stale claim in the new docstring, plus two scope questions.
Reviewed changes — the single commit d530e86, both files, plus the surrounding read paths in client/packument.rs and the tolerant deserializers in aube-registry/src/lib.rs.
- Cache writers moved to
serde_json—write_cached_packumentandwrite_cached_full_packumentnow serialize through a new privateto_cache_bytes<T: Serialize>helper; thesonic_rs::from_sliceread path is untouched. - Doc comment recording the invariant — a
serde_json::Valuemay only be handed to serde_json's own serializer, because underarbitrary_precisionNumber'sSerializeemits a private token the receiving serializer must intercept. - One unit test with a failing control —
RawValuestands in for the number token (raw_valueis on by default,arbitrary_precisionis not), and the control assertssonic_rs::to_vecleaks$serde_json::private::so the test can't quietly stop guarding the serializer choice. serde_json/raw_valuenamed in[dev-dependencies]— redundant with the workspace dep, which already sets it, so the test doesn't depend on that unification holding.
I verified the premise against the pinned versions rather than taking it on trust. sonic-rs 0.5.8's Serializer::serialize_struct intercepts only its own two tokens ($sonic_rs::private::JsonNumber, $sonic_rs::LazyValue) and falls through to serialize_map for everything else, so serde_json's tokens do leak; serde_json 1.0.150's Number::serialize does emit serialize_struct(TOKEN, 1) under arbitrary_precision; and the read side really is safe, since Value's visit_u64/visit_i64/visit_f64 are not cfg-gated and sonic-rs never routes a numeric literal through visit_map. The test's control assertion holds. arbitrary_precision is enabled nowhere on this branch, so the bug is latent here and live only in a build that unifies it in — which matches the PR body.
ℹ️ An already-poisoned cache entry still never heals, and the cache namespace was not bumped
The fix stops new poison but does not remediate existing entries, and there is no trigger that would. Once the numbers are plain maps on disk the fixed serializer writes them back verbatim, and because no read errors there is nothing to fall back to a refetch — only a genuine upstream 200 replaces the entry. Blast radius is limited to machines that ran a build unifying arbitrary_precision (today, the compile-spike branch), so this may well be out of scope, but the PR body asserts "it never heals" and then leaves it that way.
Technical details
# Poisoned packument-cache entries are not invalidated by this fix
## Affected sites
- `vendor/aube/crates/aube-registry/src/client/packument.rs:260-267` — the full path's `304` branch writes `&c.packument` straight back out; `c` is the poisoned `Value` just read, whose numbers are already `Object` nodes, so the serializer swap changes nothing.
- `vendor/aube/crates/aube-registry/src/client/packument.rs:479-512` and `846-867` — the typed-revalidate and abbreviated `304` branches have the same shape.
- `vendor/aube/crates/aube-registry/src/client/cache.rs:205-244` — `read_cached_full_packument_typed_lookup` SUCCEEDS on a poisoned file (no strict numeric field remains on `Packument`), so the `let Ok(typed) = … else { return default }` refetch escape hatch never fires.
- `vendor/aube/crates/aube-store/src/lib.rs:61-62` — `PACKUMENT_CACHE_SUBDIR = "packuments-v1"` / `PACKUMENT_FULL_CACHE_SUBDIR = "packuments-full-v1"`, unchanged by this PR.
## Required outcome
- Either a deliberate decision that poisoned entries are the affected developer's problem (and the docstring says so), or an invalidation mechanism so an existing entry is not served indefinitely.
## Suggested approach (optional)
- A subdir version bump is the cheap blunt option: the caches are regenerable and `nub` refetches on a miss.
- Narrower: have the read path treat a `$serde_json::private::` key as a cache miss, which also self-documents the failure.
## Open questions for the human
- Is remediating already-poisoned caches in scope for a fix PR, or is "clear `~/.cache/nub/packuments*`" acceptable given only dev machines can be affected?ℹ️ The same invariant is violated where nub pm use writes pnpm-workspace.yaml
I swept every non-serde_json serializer entry point in both crates/** and vendor/aube/crates/**; after this PR exactly one violation remains. crates/nub-cli/src/pm_engine/use_nub.rs:1120 hands a serde_json::Value to serde_yaml::to_string and writes the result to disk as pnpm-workspace.yaml. It is latent for the same reason the cache bug is latent, and the values come verbatim from package.json with no shape validation, so a number anywhere in overrides/auditConfig/catalogs would land as a token mapping in a real config file.
Technical details
# Remaining `serde_json::Value` → foreign serializer site
## Affected sites
- `crates/nub-cli/src/pm_engine/use_nub.rs:1120` — `serde_yaml::to_string(&serde_json::Value::Object(yaml.clone()))`, written to `pnpm-workspace.yaml` at line 1122. `yaml` is assembled at 1102-1117 from cloned `package.json` values (`workspaces`, `overrides`, `patchedDependencies`, `allowBuilds`, `auditConfig`, `catalog`, `catalogs`).
## Required outcome
- Same invariant as the packument cache: a value that reaches `serde_json::Value` is serialized by serde_json. Converting to `serde_yaml::Value` (or routing through a JSON round-trip) before handing it to the YAML serializer would satisfy it.
## Open questions for the human
- In scope for this PR, or a follow-up? Everything else checked out clean: the other `yaml_serde::to_*` / `serde_yaml::to_*` / `toml::to_*` sites serialize their own `Value` types, `aube-lockfile/src/pnpm/write.rs:953` has no `serde_json::Value` field reaching it, and the bun lockfile writer hand-rolls through `serde_json` plus `Number::to_string`.ℹ️ Nitpicks
- The comment this PR replaces documented a deliberate tradeoff ("a small throughput win on the cold-install metadata phase"). Worth a line in the PR body on whether the regression was measured for the full-packument write, where the
Valueroutinely runs to megabytes — the correctness argument stands either way, but the next person to look at this will ask.
Claude Opus ｜ 𝕏
| /// `{"$serde_json::private::Number":"27353961"}` on disk, and the next read | ||
| /// fails the typed parse with `invalid type: map, expected u64`. |
There was a problem hiding this comment.
This failure mode no longer holds on main: dist.unpackedSize is read through unpacked_size_tolerant (aube-registry/src/lib.rs:492-497), whose own docstring cites invalid type: map, expected u64 as the reason that tolerance was added — and no strict numeric field is left on Packument. The real consequence is a silent one, which makes the fix more important, not less, but the comment as written will send the next reader looking for an error that can't happen.
Technical details
# The documented consequence of a poisoned entry is stale
## Affected sites
- `vendor/aube/crates/aube-registry/src/client/cache.rs:155-156` — claims the next typed read fails with `invalid type: map, expected u64`.
- `vendor/aube/crates/aube-registry/src/lib.rs:492-497` + `513-584` — `unpacked_size_tolerant` absorbs any non-integer shape via `visit_map_to!(None)` and drops the field to `None`. Its docstring documents that exact error as the reason it exists.
- `vendor/aube/crates/aube-registry/src/client/cache.rs:209-224` — the typed reader's `Typed` struct therefore parses a poisoned file successfully, so its refetch escape hatch never fires.
## Required outcome
- The comment describes what actually happens: the poisoned entry parses, `unpackedSize` silently becomes `None`, the three `Option<serde_json::Value>` trust fields carry the token map, and nothing errors — so nothing triggers a refetch.
## Suggested approach (optional)
- Worth adding, since it is the non-obvious half: under `arbitrary_precision` the poison round-trips losslessly, because `KeyClassifier` (serde_json `src/value/de.rs`) maps the token back to `KeyClass::Number` and `ValueVisitor::visit_map` reconstructs `Value::Number`. The build that writes the corruption cannot observe it; only a build without the feature — the released binary — sees a map.| /// `{"$serde_json::private::Number":"27353961"}` on disk, and the next read | |
| /// fails the typed parse with `invalid type: map, expected u64`. | |
| /// `{"$serde_json::private::Number":"27353961"}` on disk. Nothing fails loudly | |
| /// on the way back in: `unpacked_size_tolerant` absorbs the map shape and drops | |
| /// the field to `None`, so the entry parses and the corruption stays silent. |
|
Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0 |
Both packument-cache writers serialize through
serde_json::to_vecinstead ofsonic_rs::to_vec; read path unchanged.A
serde_json::Valuemay only be serialized by serde_json. Underarbitrary_precision, sonic-rs writesNumber's private token literally, so cached numbers land as{"$serde_json::private::Number":"…"}and the next typed read failsinvalid type: map, expected u64. Revalidation rewrites the poison verbatim, so it never heals.nub compilepullsrolldown_common, which requires that feature; Cargo unifies per build and the cache is shared. No released binary can produce it. Also silently corruptstrustedPublisher/approver.fmt, clippy, 309 tests pass.