mirror of
https://github.com/pnpm/pnpm.git
synced 2026-07-20 04:32:26 -04:00
b7367e34f1
* feat(store): decode pnpm-written msgpackr-records rows in index.db Finishes the last open item on #244. pnpm packs every PackageFilesIndex with `new Packr({ useRecords: true, moreTypes: true })`. Records use a msgpackr-private extension (ext type 0x72, "r") that rewrites the string keys of repeated same-shape structs into 1-byte slot references, so the payload `rmp_serde` saw in a pnpm-written index.db row was effectively garbage — the cache-lookup in `StoreIndex::get` would error, pacquet would fall through to a fresh download, and the whole "shared v11 store" promise evaporated for rows pnpm wrote first. - New `crates/store-dir/src/msgpackr_records.rs`: a focused transcoder that walks a msgpackr-records byte stream and emits equivalent plain MessagePack (expanding each record instance into a string-keyed map). Keeping this as a transcoder — rather than a fresh Deserializer — lets us reuse the existing `Deserialize` derive for `PackageFilesIndex`. 270 lines of decoder + 10 unit tests using real fixture bytes captured from msgpackr 1.11.8 (via `store/index/node_modules/msgpackr` in the pnpm v11 worktree). - `StoreIndex::get` now sniffs the leading `d4 72` bytes and routes msgpackr-records rows through the transcoder before handing the result to `rmp_serde`. Pacquet-written rows (plain msgpack) still take the zero-copy fast path — no transcoding, no extra allocation. - The transcoder also narrows integer-valued `float 64`/`float 32` values to `uint 64`. Msgpackr emits JS Number as float whenever the value exceeds int32 range, so millisecond timestamps like `checkedAt = 1_700_000_000_000` arrive as a double even though they're semantically ints — and `rmp_serde` rejects floats for our integer-typed `size: u64` / `checked_at: Option<u128>` fields. Non-integer floats (π and friends) pass through unchanged. - Drops `#[ignore]` on `crates/cli/tests/pnpm_compatibility::same_index_file_contents`. End-to-end: `pnpm install` writes rows, pacquet reads them back, and the snapshot of decoded contents now matches what pacquet itself would have written. Refs #244. * fix(store): make pacquet-written rows decodable by pnpm too Symmetric follow-up to the previous commit: with just msgpackr-records decoding in place, pacquet can read what pnpm wrote, but pnpm could not reliably read what *pacquet* wrote. Verified empirically by feeding pacquet's output to `new Packr({useRecords: true, moreTypes: true})` from the pnpm v11 workspace's msgpackr 1.11.8. Two issues surfaced, both in how pacquet encoded `CafsFileInfo`: 1. `checked_at: Option<u128>` — `rmp_serde` has no native MessagePack integer encoding for `u128` and fell back to writing the value as a `bin 16` (16 raw big-endian bytes). msgpackr decoded the field as a `Uint8Array`, not a timestamp, so pnpm's integrity check would have silently misbehaved. Narrow the type to `u64` — millisecond timestamps fit in `u64` until year ~584M, and `u64` maps cleanly to MessagePack's integer encodings. 2. Even after the u128 → u64 fix, writing as `uint 64` (`cf`) made msgpackr return the value as a JS `BigInt`. pnpm's CAFS integrity check does `mtimeMs - (checkedAt ?? 0)`; mixing `Number` and `BigInt` throws `TypeError: Cannot mix BigInt and other types` at runtime. Pnpm itself sidesteps this because JS Number is a double — msgpackr packs large integer-valued Numbers as `float 64` (`cb`). Match that: add a `serialize_with` that emits `checked_at` as `float 64`. Verified: msgpackr now returns the value as `typeof === 'number'`, bit-identical byte encoding to what pnpm itself produces. Supporting read-side changes: - Add `deserialize_with` on `checked_at` that accepts either integer or integer-valued float. Required because plain msgpack reads bypass the records transcoder (the transcoder would reinterpret legitimate positive fixints in 0x40..=0x7f as record slot references, which is correct under records mode but wrong for pacquet-written rows). - Restore the sniff on `decode_index_value` — only invoke the transcoder when the bytes begin with the msgpackr records marker `d4 72`. An earlier version of this commit tried "always transcode" for simplicity; it broke every pacquet-written row whose `size` or `mode` happened to land in the 0x40..=0x7f byte range. Test updates: - `round_trips_plain_msgpack_through_transcoder` replaces `passes_through_plain_msgpack_unchanged`: the byte-for-byte guarantee no longer holds (the transcoder now narrows integer-valued floats), but the decoded-struct round-trip still does. - Add `plain_msgpack_without_floats_passes_through_unchanged` to pin down the narrower pass-through guarantee for non-float inputs. No changes to `tarball/src/lib.rs` beyond the `as_millis() as u64` cast the u128 → u64 narrowing now requires. * refactor(store): records-mode tracking + tight float upper bound Copilot review round one on #251. - Track whether the transcoder has seen a record definition yet, and only treat `0x40..=0x7f` as slot references once records mode is on. Until then those bytes are positive fixints (64..=127), the same as vanilla MessagePack. Before this change the transcoder couldn't be run on plain-msgpack input without corrupting it — pacquet rows with `size: 123` or similar would be mis-read as a reference to an undefined slot 0x7b. The old code sidestepped the problem with a sniff in `decode_index_value` (transcode only when bytes start with `d4 72`); records-mode tracking makes the transcoder safe for both encodings, so `decode_index_value` drops the sniff and always transcodes (which is necessary anyway for the float-to-uint narrowing on pacquet's own `checkedAt` field). - Replace the `v <= u64::MAX as f64` narrowing bound with a strict `v < 2f64.powi(64)`. `u64::MAX as f64` rounds *up* to 2^64 because 2^64 - 1 isn't exactly representable in `f64`, so the old bound accepted a literal 2^64 and silently saturated it to `u64::MAX` on cast. - Add `plain_positive_fixint_in_slot_range_passes_through` and `float64_equal_to_2_pow_64_passes_through` as regression guards for both fixes. - Drop the now-redundant custom `deserialize_with` on `CafsFileInfo::checked_at` — since all reads transcode first and the transcoder narrows integer-valued floats back to `uint 64`, the value always arrives as an integer for `rmp_serde` to deserialize. Refs #244. * refactor(store): sharper error variants + unreachable-slot guard + micro-tweaks Copilot review round two on #251. - Split `DecodeError::BadRecordDef` into three precise variants — `ExpectedArrayHeader`, `ExpectedStringHeader`, and `InvalidFieldNameUtf8`. The old shared variant carried a meaningless `count: 0 | 1` indicator that made messages like "Expected record-definition header (0 field-name strings in a msgpack array)" actively confusing. - Add `DecodeError::SlotOutOfRange` and reject any record definition whose slot byte isn't in `0x40..=0x7f`. msgpackr never emits one outside that range; accepting them before meant the slot was registered but unreachable, so the first reference-attempt elsewhere in the stream would have surfaced as an unrelated `UnknownSlot` error. - Drop three needless `to_vec()` allocations on the array16, array32, map16, and map32 header-copy paths — use `w.extend_from_slice(r.read_bytes(n)?)` directly. - Switch the `as_millis() as u64` cast in tarball extraction to `u64::try_from(...).ok()` so a hypothetical u128-overflowing timestamp drops to `None` instead of silently wrapping. - Fix a stale doc comment that still referred to an `Option<u128>` deserializer (now `Option<u64>`). - Rewrite the `StoreIndex::get` doc so it matches the current always-transcode implementation — no more "sniff the leading two bytes" language now that records-mode tracking makes unconditional transcoding safe. Refs #244. * refactor(store): Rc<[String]> slot schemas + correct get() doc Respond to two Copilot review notes: - Slot-reference decoding used to `.clone()` the full Vec<String> of field names per record instance. A 200-file row allocated 200 Vec<String>s plus one String per field per clone. Store the slot schema under `Rc<[String]>` so the reference path bumps a refcount instead. Single-thread transcoder, so `Rc` over `Arc`. - `StoreIndex::get` had a doc comment claiming a "sniff the leading two bytes" fast path that didn't exist — the implementation always transcodes. Explain why (pacquet writes `checkedAt` as float64 on purpose for msgpackr/pnpm interop, so every real row needs the transcoder to narrow it back), so the always-transcode behaviour reads as deliberate, not a missing optimisation. * refactor(store): consistent diagnostic codes + format-agnostic EOF msg Two Copilot nits: - Diagnostic codes said `pacquet_store_dir::msgpackr::…` even though the module is `msgpackr_records`. Match the `store_index::…` shape used elsewhere in the crate so codes map 1:1 to module paths. - `UnexpectedEof` said "msgpackr buffer", but the transcoder also runs on plain MessagePack (see the `get_decodes_…` path through `StoreIndex::get`). Reword to "MessagePack buffer" so the error reads correctly in either case.