--- status: resolved (F1, F2 fixed 2026-09-07; N1, N2, N3 classified; N4 fixed 2026-09-07) last_updated: 2026-09-07 reviewed_artifacts: - src/read_plan.rs - src/sequential_reader.rs - src/materialize.rs - src/bast.rs - benches/wire_vs_bast.rs - docs/reviews/007-coverage-audit.md (N3a disposition) tool: manual diff review of post-#007 commits (dea96f0, d4635d2) + disposable probe tests (run in-session, then deleted) + cargo bench + counting-allocator peak-RSS probe reviewer: pre-publish review #008 (session request — audit the two post-#007 perf/bench commits, then gate the 0.3.0 publish) --- # Review #008 — Pre-Publish Review: Post-#007 Perf Commits ## Purpose 0.3.0's release commit (`9949f91`) predates review #006 entirely; the fix sessions for #006 and #007 landed eleven more commits, and *after* #007 closed, two more commits landed unreviewed: `dea96f0` (bench port from alktty) and `d4635d2` (the perf commit — fixed-size struct fast path, integer union dispatch, `read_next_borrowed`). The perf commit touches the flagship packed read path, which every untrusted wire buffer flows through. This review audits those two commits before the first crates.io publish of the 0.3.x line (0.1.0 and 0.2.0 are published; 0.3.0 never was — every post-release fix can legally ride inside the first published 0.3.0, no semver conflict). It also disposes of review #007's N3a — the one finding explicitly deferred to "the pre-release review", which this session is. ## Methodology - Full diff read of `d4635d2` (perf) and `dea96f0` (bench port), cross-checked against the invariants the earlier reviews established: cross-consumer dispatch agreement (#006 H3, #007 F1), the H1 no-count-sized-prealloc rule, and the N2/F2 dual-layer cap pattern. - Disposable probe tests (`tests/zzz_probe*.rs`, deleted after the session; none was a crash hazard) to confirm/deny the three behaviors code reading flagged: the `int_keys` non-canonical-key divergence, the engine's nested-array acceptance envelope, and the `with_capacity` amplification. - A counting-`GlobalAlloc` probe (peak-bytes metric) to measure the worst-case simultaneous allocation of the amplification shape precisely — RSS timing proved too noisy to separate the two test cases. - `cargo bench --quick` before/after the fixes to confirm the perf commit's wins survive. - N3a dispositions probed where cheap (inline-struct union variants). ## Baseline Audited at `main` HEAD `d4635d2`, 0.3.0, working tree clean. 566 tests green (488 lib + 17 + 34 + 15 + 12, + 2 ignored doctests), clippy `-D warnings` clean, wasm build green — per the perf commit's verification block. ## Summary Statistics | Severity | Count | Status | |----------|------:|--------| | High | 0 | — | | Medium | 2 (F1, F2) | both fixed 2026-09-07 | | Info | 3 (N1, N2, N3) | classified | | Fix | 1 (N4) | fixed 2026-09-07 | No Highs: both Mediums are probe-verified cross-consumer divergences and resource-bound violations, but neither aborts the process (`with_capacity` is now bounded per array by `MAX_ARRAY_ELEMENTS`, so H1's 1 TB SIGABRT class does not return). Both were fixed in-session because they violate AGENTS.md §3 (divergent verdicts on untrusted input; unbounded-count-shaped allocation) — the publish gate. **Resolution log:** - **F1 + F2 (2026-09-07):** fixed in one commit — see the resolution blocks. 569 tests green (491 lib + 17 + 34 + 15 + 12, + 2 ignored), clippy `-D warnings` clean, doc 0 warnings, wasm green. - **N4 (2026-09-07):** fixed with F1/F2 — see the block. --- ## Findings ### F1. `int_keys` integer dispatch breaks cross-consumer agreement on non-canonical mapping keys **Files**: `src/read_plan.rs` (`compile_int_keys`, introduced by `d4635d2`), contrast `src/materialize.rs` (`materialize_plan_union`'s byte-disc arm — stringifies), `src/validation_plan.rs` (`validate_union_numeric` — stringifies), `src/tunion.rs` (`read_byte_discriminator` — stringifies) **Problem**: `d4635d2` added a pre-parsed `(u64, variant_index)` dispatch table for byte-discriminator unions: when every mapping key parses as `u64`, the reader matches the raw discriminator integer instead of stringifying per read. But the meta-schema does not constrain mapping-key shape beyond "object property name", and `key.parse::()` accepts **non-canonical** decimal strings: ``` PROBE1 reader: field=msg disc="01" (DISPATCHED) PROBE1 validate_bytes: Err(access error at msg: union discriminator value 1 not in mapping) ``` With mapping key `"01"` (and discriminator `1` on the wire): the reader's numeric dispatch **matches** (`"01".parse::() == 1`) and dispatches — returning `discriminator == "01"` — while the materializer (`1.to_string() == "1" ≠ "01"`), the validation plan, and tunion all **reject** the identical buffer. Pre-`d4635d2`, all four consumers stringified and all four rejected — agreement held (both verdicts "reject", same error class). The perf commit flipped the reader to accept-while-everyone-else-rejects: the exact cross-consumer-divergence shape #006 H3 and #007 F1 exist for, on the flagship path. `"+1"` parses as `u64` too (Rust's `from_str_radix` accepts a leading `+`) — same class. The returned key string also became schema-quirk-dependent: the reader reports `"01"` where the materializer's `__discriminator` for a *matched* key would report the stringified form. **Not a #007 regression**: `int_keys` did not exist before `d4635d2`. But `d4635d2` postdates #007's close and was unreviewed — this is the audit catching it. **Fix**: build the integer table only from **canonical** keys — a key qualifies iff `key.parse::()` succeeds *and* `parsed.to_string() == key` (i.e. the key is exactly what stringification would produce). Any non-canonical or non-numeric key falls back to the string path (`int_keys: None`), which every consumer already agrees on. No accepted schema's *reachable* behavior changed: for fully-canonical mappings the numeric dispatch behaves identically to stringified matching (the numeric value's `to_string()` equals the key), and for non-canonical keys all consumers now reject exactly as before `d4635d2`. The perf win (no per-read stringify) is preserved for every mapping that was unambiguous to begin with. **Resolution (2026-09-07):** exactly that — `compile_int_keys` now requires `v.to_string() == *key` for the table to carry the entry; any miss returns `Ok(None)` (string fallback). Doc comment states the canonicality rule and why. Tests in `read_plan.rs`: `r8_non_canonical_mapping_key_disables_int_dispatch` (key `"01"` → `int_keys` is `None`) and `r8_canonical_mapping_keys_keep_int_dispatch` (keys `"1"`,`"2"` → table `[(1,0),(2,1)]`). Probe output after the fix: both `validate_bytes` and the reader reject disc 1 under key `"01"` with the same error class — agreement restored. ### F2. `Vec::with_capacity(count)` reintroduced on both array materializers — ~477 MB simultaneous allocation from a ~1 KB schema **Files**: `src/materialize.rs:247` (`materialize_plan_array` — the `validate_bytes` packed path), `src/materialize.rs:650` (`materialize_array_packed` — the legacy walker, reachable via the aligned record arm) **Problem**: `d4635d2`'s "materialize: with_capacity for bytes arrays, arrays, and struct objects" item reintroduced `Vec::with_capacity(count)` at two of the three sites H1's layer-1 fix had converted to `Vec::new()` + push. `count` is now compile-capped at `MAX_ARRAY_ELEMENTS` (2^16), so H1's 1 TB `SIGABRT` does not return — but the per-array cap does not bound *nesting*: ``` PROBE validate peak bytes allocated simultaneously: 476780249 ``` A schema of one 100-level nested array chain (each `count: 65535`, innermost elements empty structs — all legal: the depth cap is 128 and stride-0 chains evade `MAX_ARRAY_BYTES`, which only checks stride products) peaks at **~477 MB of simultaneous allocation** on `validate_bytes(&[])` from a ~1 KB schema and an *empty* buffer. Each level's `with_capacity(65535 × sizeof(Value))` stays live across its element walk, so the sizes multiply across ~127 legal depth levels (the innermost zero-progress rejection fires only after the whole chain has descended). On wasm32 — which this crate explicitly targets — the same shape aborts the wasm heap well below 477 MB. H1's layer-1 rule ("no count-sized prealloc on untrusted input; the per-element walk dominates") is exactly the invariant this violates; the perf commit's own bench evidence doesn't need the prealloc either (see below). The other `with_capacity` additions in the commit are fine: byte-array capacity from `b.len()` (a read slice), struct-object capacity from `plan.fields().len()`, and the plan-compiler's from `fields.len()` — all bounded by data/plan already in hand, not by declared counts. **Fix**: restore H1's layer-1 shape at both sites — `Vec::new()` + push loop (the loops already push `count` elements; the zero-progress guard bounds honest progress per element). Optionally cap preallocation at a small constant, but plain `Vec::new()` matches H1's shipped behavior. **Resolution (2026-09-07):** both sites restored to `Vec::new()` + push. Bench before/after (criterion `--quick`, this session): packet read 246 → 220 µs, chunk read 76/68 µs — the revert costs nothing measurable on the bench shapes (small arrays; the materializer's per-element work dominates), and the union/struct preallocs stay. Locking test in `materialize.rs`: `r8_deeply_nested_stride0_array_rejects_before_bulk_prealloc` (the 100-level chain still rejects cleanly with the zero-progress error at the innermost level; the allocation shape itself is documented here — in-tree cannot cheaply assert peak allocation, and the #008 probe was deleted per the no-reproducer rule). ### N1. `d4635d2`'s fixed-size fast paths are sound (classified, no action) **Files**: `src/read_plan.rs` (`fixed_size`, `fixed_plan_size`), `src/sequential_reader.rs` The fixed-size struct fast path replaces the cursor size walk with one bounds check; `fixed_plan_size` already returned `Result