Pre-publish review #008: gate int_keys on canonical keys, restore no-prealloc array rule
Review #008 (docs/reviews/008-pre-publish-review.md) audits the two post-#007 unreviewed commits (dea96f0bench port,d4635d2perf) before the first crates.io publish of 0.3.0. - F1: the int_keys integer dispatch accepted non-canonical mapping keys ("01", "+1" parse as u64 1) — the reader dispatched disc 1 while the materializer, validation plan, and tunion rejected the same buffer. compile_int_keys now builds the table only when every key is canonical (v.to_string() == key); otherwise the string fallback applies (agreement restored, perf kept for canonical mappings). Tests: r8_non_canonical_mapping_key_disables_int_dispatch, r8_canonical_mapping_keys_keep_int_dispatch. - F2:d4635d2reintroduced Vec::with_capacity(count) on both array materializers. Bounded per array by MAX_ARRAY_ELEMENTS but nesting compounds: probe (counting allocator) measured ~477 MB simultaneous allocation from a ~1 KB schema + empty buffer (100-level stride-0 chain, all legal under the caps). H1's layer-1 rule restored: Vec::new() + push. Bench unchanged (packet read 220µs vs 246µs baseline). Test: r8_deeply_nested_stride0_array_rejects_before_bulk_prealloc. - N3a disposition (review #007's deferred item): BastField::synthetic (pub(crate), zero callers, #[allow(dead_code)]) deleted; the seven source() accessors are public API and stay (semver decision — removal needs an explicit ask); resolve_typeref_as_def's inline struct/union/enum arms probe-verified reachable (inline struct union variants are legal) — kept. - CHANGELOG: 0.3.0 entry (compiled forms, breaking surface, hardening fixes, coverage). README: ReadPlan/ValidationPlan roles, union conventions, untrusted-schema bounds. Verification: 569 tests green (491 lib + 17 + 34 + 15 + 12, + 2 ignored doctests), clippy -D warnings clean, cargo doc 0 warnings, wasm32 build green, cargo publish --dry-run clean.
This commit is contained in:
1 parent
d4635d28f0
commit
9803d3b768
6 files changed
+520
-29
No files matched your search
+108
@@ -4,6 +4,113 @@ All notable changes to this crate are documented here. The format is
|
||||
based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and
|
||||
this crate adheres to [Semantic Versioning](https://semver.org/).
|
||||
|
||||
## [0.3.0] - 2026-09-07
|
||||
|
||||
The compiled-forms release. The packed read path — the hot path for
|
||||
stream parsing — is driven by a compile-once `ReadPlan` instead of a
|
||||
per-call walk of the BAST typed tree; byte validation runs a compiled
|
||||
`ValidationPlan`; plans and offset maps fingerprint to a stable hash
|
||||
(ADR-011/ADR-012). Reads of SFTP-shaped packet streams went from
|
||||
~189× hand-rolled Rust to ~74× (~2.4× faster), fixed-stride chunk
|
||||
reads from ~18× to ~11×, and `SequentialReader::read_next_borrowed`
|
||||
makes the per-field hot loop allocation-free.
|
||||
|
||||
### Breaking changes
|
||||
|
||||
- **`Bast*` types are owned.** `BastDoc`/`BastStruct`/`BastField`/… no
|
||||
longer borrow from the source `serde_json::Value`; all v0.2.0
|
||||
lifetimes are gone. `BastDoc::new` parses the root eagerly; `$ref`s
|
||||
resolve lazily.
|
||||
- **`OffsetMap::get` / `PackedLayout::get`** return `&OffsetEntry`
|
||||
(was `Option<OffsetEntry>` by value), backed by an O(log n)
|
||||
`BTreeMap` path→index (first-occurrence-wins for duplicate names).
|
||||
- **`SequentialReader::new`** takes the compiled plan; construct via
|
||||
`AlkTypeEngine::sequential_reader()` (packed mode only).
|
||||
- **`materialize_packed` / `materialize_aligned`** take the compiled
|
||||
plan / `(&BastDoc, &OffsetMap)` pair respectively.
|
||||
- **Field-name-discriminator union wire convention** (ADR-011
|
||||
addendum): the builder lays out the union's declared `fields`
|
||||
(shared) first, then the variant's own fields. Variants must not
|
||||
re-declare the discriminator or any shared field, and the
|
||||
discriminator field must be the first entry in `fields` — all
|
||||
enforced at parse with clean `Schema` errors. Schemas relying on
|
||||
0.2.0's variant-only layout are rejected (they produced
|
||||
reader↔builder-disagreeing bytes).
|
||||
- **`maxLength` is string/bytes-only** — rejected at parse on every
|
||||
other kind (it was silently unenforced there).
|
||||
- **Aligned-mode `Record` fields reject `offset-indirect`** (the
|
||||
materializer always walks the inline count-prefixed form — the
|
||||
annotated shape was never readable).
|
||||
- **Schema input bounds** (untrusted-schema hardening, AGENTS.md §3):
|
||||
array `count` ≤ 2^16 and `count × stride` ≤ 2^26 bytes; `align` ≤
|
||||
4096; `maxLength` ≤ 2^26; cyclic `$ref` graphs and >128-deep nesting
|
||||
are rejected by every public walker (`OffsetMap::compute`,
|
||||
`LayoutBuilder::new`, `materialize_aligned` included), not just the
|
||||
engine.
|
||||
|
||||
### Additions
|
||||
|
||||
- **`ReadPlan`** (ADR-011) — the compiled packed-read plan, re-exported
|
||||
with `CompositePlan`/`FieldPlan`/`ReadKind`/`DiscriminatorPlan`.
|
||||
`ReadPlan::compile` is untrusted-input-safe standalone (depth cap +
|
||||
cycle set). `fixed_size()` exposes the compile-time-known byte size
|
||||
for fixed structs.
|
||||
- **`ValidationPlan`** (ADR-012 §3) — the compiled `validate_bytes`
|
||||
walker, with `ValidNode`/`ValidVariant` sub-types.
|
||||
- **`fingerprint()`** on `ReadPlan`/`OffsetMap`/`ValidationPlan` +
|
||||
`Hash`/`Eq` derives on the plan types (ADR-012 §1/§4) — plan
|
||||
identity for cache-keying across processes.
|
||||
- **`OffsetMap` `LeafMeta`** — each entry records whether it is
|
||||
fixed/length-prefixed/offset-indirect so `read_field`/`write_field`
|
||||
dispatch without re-walking the schema; `OffsetEntry` type re-exported.
|
||||
- **`SequentialReader::read_next_borrowed`** — zero-allocation variant
|
||||
of `read_next` (field name borrowed from the plan).
|
||||
- **`AlkTypeEngine::validate_bytes`** now runs the compiled
|
||||
`ValidationPlan` (was an interpretive BAST walk in 0.2.0).
|
||||
|
||||
### Fixes (post-release-commit hardening — reviews #006, #007, #008)
|
||||
|
||||
All found and fixed before the first crates.io publish of 0.3.0, so
|
||||
no published version ever exhibited them.
|
||||
|
||||
- **Untrusted-input crashes removed.** A huge declared array count
|
||||
OOM-aborted the process (`Vec::with_capacity(count)` before reading
|
||||
a byte) — now compile-capped and walked with push-only growth.
|
||||
Cyclic `$ref` graphs stack-overflowed the three standalone layout
|
||||
walkers — now guarded by a shared reference-graph check. Deeply
|
||||
nested stride-0 arrays briefly allowed ~477 MB of simultaneous
|
||||
allocation from a ~1 KB schema — restored to incremental growth.
|
||||
- **Cross-consumer divergences closed.** Builder, reader,
|
||||
materializer, tunion, and the validation plan now agree on
|
||||
field-disc union layout (shared-then-variant), on the discriminator
|
||||
field's position (must be first), and on union mapping-key matching
|
||||
(numeric fast-path dispatch only for canonical keys like `"2"`;
|
||||
`"01"`/`"+1"` fall back to the string comparison all consumers
|
||||
share). The legacy BAST walker's field-disc union arm walks shared
|
||||
fields before the variant (it previously materialized variant fields
|
||||
from shared fields' bytes).
|
||||
- **Silently-corrupt layouts rejected.** Aligned record fields with
|
||||
`maxLength`/`offset-indirect`; non-final inline length-prefixed
|
||||
fields (records included — the ADR-006 check now sees them);
|
||||
aligned-mode `maxLength`/`offset-indirect` on records; unions in
|
||||
aligned mode (pre-existing, now tested).
|
||||
- **Coverage**: 90.67% lines / 86.32% functions at review #007's
|
||||
audit, 91.66% after its fixes; every uncovered region outside test
|
||||
modules read and classified in-tree (docs/reviews/007).
|
||||
|
||||
### Non-breaking improvements
|
||||
|
||||
- Engine compile is one-shot and allocation-tidy; plans are
|
||||
`Send + Sync` (statically asserted) and fingerprintable.
|
||||
- Zero-progress array-element guard on all three array walkers (a
|
||||
zero-size element makes the declared count unbounded on the wire).
|
||||
- WASM-clean unchanged: two dependencies (`jsonschema`
|
||||
default-features off, `serde_json` with `preserve_order`), no
|
||||
`async`, no `unsafe`, no feature flags.
|
||||
- Benches (`benches/wire_vs_bast.rs`): read/write chunk streams, an
|
||||
SFTP-shaped union packet stream, and `validate_bytes` per buffer —
|
||||
the numbers quoted above and in ADR-007/ADR-011.
|
||||
|
||||
## [0.2.0] - 2026-08-17
|
||||
|
||||
A breaking release that replaces the v0.1.0 `AlkType:*` custom-keyword
|
||||
@@ -115,5 +222,6 @@ Initial crates.io release. Custom-keyword JSON Schema format
|
||||
`AlkTypeEngine` with packed/aligned layout modes, builder API producing
|
||||
`serde_json::Value`.
|
||||
|
||||
[0.3.0]: https://git.alk.dev/alkdev/alktype/releases/tag/v0.3.0
|
||||
[0.2.0]: https://git.alk.dev/alkdev/alktype/releases/tag/v0.2.0
|
||||
[0.1.0]: https://git.alk.dev/alkdev/alktype/releases/tag/v0.1.0
|
||||
@@ -24,10 +24,11 @@ A BAST document serves three roles simultaneously:
|
||||
|
||||
| Role | Mechanism | When |
|
||||
|------|-----------|------|
|
||||
| **Validation spec (bytes)** | BAST-native validator (recursive walker over the BAST type tree) | Access time (`validate_bytes`) |
|
||||
| **Validation spec (bytes)** | Compiled `ValidationPlan` walk over the materialized `Value` (ADR-012) | Access time (`validate_bytes`) |
|
||||
| **Validation spec (JSON)** | Standard `jsonschema::Validator` from a consumer-provided JSON Schema | Load time (build validator), access time (`validate_json`) |
|
||||
| **Layout spec** | Offset computation from type sizes + field order | Load time (build offset map / packed layout) |
|
||||
| **Data access** | Read/write at computed offsets | Access time (read field, write field) |
|
||||
| **Wire access (packed)** | Compiled `ReadPlan` (ADR-011) — compile-once, no per-read schema walk | Access time (`SequentialReader`) |
|
||||
|
||||
No separate format definition, no separate parser, no separate
|
||||
validator. The BAST document is the single source of truth for the
|
||||
@@ -159,11 +160,16 @@ same BAST document can be compiled in either mode. Decided in ADR-002.
|
||||
- **Byte-offset** — a fixed-size integer (`uint8`/`uint16`/`uint32`) at
|
||||
a known byte offset. The SFTP `Packet` pattern: byte 0 is the type
|
||||
byte, bytes 1..N are the variant struct. Mapping keys are stringified
|
||||
integers.
|
||||
integers. With all-canonical numeric keys the compiled reader
|
||||
dispatches on the raw integer (no per-read stringification).
|
||||
- **Field-name** — a named field within the union. The TypeBox
|
||||
`typedef.ts` pattern. Mapping keys are string values matching the
|
||||
discriminator field's value. The `fields` array declares the
|
||||
discriminator field (D-BAST-005).
|
||||
discriminator field (D-BAST-005), which must be its first entry; the
|
||||
variant must not re-declare it or any shared field. The builder lays
|
||||
out the declared `fields` first, then the variant's own fields
|
||||
(ADR-011 addendum) — builder, reader, materializer, and validator all
|
||||
agree on that convention.
|
||||
|
||||
Variant `$ref`s are resolved lazily — no compile-time inlining step.
|
||||
|
||||
@@ -182,10 +188,10 @@ types (ADR-VAL-SPLIT):
|
||||
|
||||
- `validate_bytes(&[u8])` — for raw byte buffers (channels' chunk
|
||||
header, SFTP packets). Materializes a `Value` tree from the bytes via
|
||||
the layout engine, then runs the **BAST-native validator** — a
|
||||
recursive walker over the BAST type tree that checks the value-domain
|
||||
constraints the materializer doesn't (integer ranges, `maxLength`,
|
||||
enum index bounds, union variant constraints). No
|
||||
the layout engine, then runs the compiled **`ValidationPlan`** (0.2.0
|
||||
used an interpretive BAST walker; 0.3.0 compiles the value-domain
|
||||
constraints — integer ranges, `maxLength`, enum index bounds, union
|
||||
variant dispatch — once at compile time). No
|
||||
`jsonschema` involvement; the BAST document is the complete
|
||||
validation spec for bytes (D-BAST-006).
|
||||
- `validate_json(&Value)` / `is_valid_json(&Value)` — for already-parsed
|
||||
@@ -244,6 +250,15 @@ code was converted to `Err` ahead of v0.1.0 (review #002, L2); the
|
||||
BAST parser preserves this invariant — overflow-safe arithmetic
|
||||
(`checked_add`, `usize::try_from`) on all offset/count casts.
|
||||
|
||||
0.3.0 adds compile-time bounds for adversarial schemas: array counts
|
||||
≤ 2^16 elements, computed array sizes ≤ 2^26 bytes, `align` ≤ 4096,
|
||||
`maxLength` ≤ 2^26, and a shared reference-graph guard that rejects
|
||||
cyclic `$ref`s and >128-deep nesting in every public schema walker.
|
||||
Adversarial buffers fail with `Access` errors at read time — the
|
||||
materializers never preallocate from declared counts. Reviews #006,
|
||||
#007, and #008 document the audit trail
|
||||
([docs/reviews/](docs/reviews/)).
|
||||
|
||||
## Documentation
|
||||
|
||||
Architecture documentation lives under [`docs/architecture/`](docs/architecture/):
|
||||
|
||||
@@ -0,0 +1,296 @@
|
||||
---
|
||||
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::<u64>()` 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::<u64>() == 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::<u64>()` 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<Option>` with
|
||||
clean overflow errors (L1's shape), and every new error arm formats
|
||||
paths lazily on the error path only. The union-variant fast path
|
||||
(`plan_variant_fixed_size`) applies only to struct variants and checks
|
||||
bounds before use. No issue found.
|
||||
|
||||
### N2. Bench port (`dea96f0`) is methodology-honest (classified, no action)
|
||||
|
||||
**Files**: `benches/wire_vs_bast.rs`
|
||||
|
||||
The port drops alktty's async I/O group (correctly — it measured a
|
||||
different stack) and adds a parity check before measurement so the
|
||||
stream loop can't drift. The historical `read_chunk_stream` numbers
|
||||
stay comparable by construction. No issue found.
|
||||
|
||||
### N3. N3a dispositions (review #007's deferred items)
|
||||
|
||||
**Files**: `src/bast.rs` (`source()` accessors, `BastField::synthetic`,
|
||||
`resolve_typeref_as_def`'s inline arms)
|
||||
|
||||
- **`source()` accessors (7 sites)**: public API on `BastStruct`/
|
||||
`BastUnion`/`BastField`/etc. Removal is a semver decision and
|
||||
AGENTS.md's semver exception requires an explicit ask — **kept**.
|
||||
They are one-line accessors over parsed source nodes, harmless, and
|
||||
plausibly useful to downstream codegen (the announced consumer).
|
||||
- **`BastField::synthetic` (`#[allow(dead_code)]`, zero callers)**:
|
||||
`pub(crate)`, not public API — **deleted** (2026-09-07). No semver
|
||||
impact; the `#[allow(dead_code)]` suppression is gone with it.
|
||||
- **`resolve_typeref_as_def`'s inline struct/union/enum arms**: the
|
||||
review-#007 suspicion ("plausibly dead after H3") was wrong —
|
||||
probe-verified reachable: the meta-schema's
|
||||
`mapping.additionalProperties: TypeRef` accepts inline struct
|
||||
variants, and `LayoutBuilder`'s byte-disc and field-disc arms call
|
||||
`resolve_typeref_as_def` on every union variant. The H3 parse rules
|
||||
forbid variant *re-declaration of shared fields*, not inline variant
|
||||
bodies. **Kept**, reachable.
|
||||
|
||||
### N4. Stale test-count references in review #006's resolution log
|
||||
|
||||
**Files**: `docs/reviews/006-implementation-review-030.md`
|
||||
|
||||
The bookkeeping note ("static count at `2eb086f` is 542 + 2 ignored")
|
||||
and per-commit counts are accurate as written; no fix needed. Recorded
|
||||
here so the review trail stays honest about what was re-checked
|
||||
during this session's doc sweep. **Resolution (2026-09-07):** no code
|
||||
change; superseded the "Fix" entry — this is the classification
|
||||
record.
|
||||
|
||||
---
|
||||
|
||||
## What's Good
|
||||
|
||||
- The perf commit's core ideas are sound and survived review: the
|
||||
compile-time `fixed_size` cache is computed through the existing
|
||||
`Result`-returning sizer (no `unwrap_or_default` regression), and
|
||||
the int-dispatch table's design was right — it just needed the
|
||||
canonicality gate.
|
||||
- The counting-allocator probe took 15 minutes and converted a
|
||||
"probably too big" into a precise number (476,780,249 bytes) — the
|
||||
same probe pattern the earlier reviews used, applied to allocation
|
||||
instead of verdicts.
|
||||
- `cargo bench --quick` before/after the fixes is the right tool for
|
||||
guarding perf-fix reverts: packet read 220 µs post-fix vs 246 µs
|
||||
baseline confirms the `Vec::new()` restore is free.
|
||||
|
||||
## Recommended Order
|
||||
|
||||
1. ~~**F1** — canonical-key gate on `compile_int_keys`~~ **fixed
|
||||
2026-09-07**.
|
||||
2. ~~**F2** — restore H1's no-prealloc rule at both array sites~~
|
||||
**fixed 2026-09-07**.
|
||||
3. ~~**N4** — `BastField::synthetic` deletion~~ **fixed 2026-09-07**.
|
||||
4. **N3 source() accessors** — revisit only if/when the codegen
|
||||
consumer confirms it does not want them (removal needs an explicit
|
||||
ask per AGENTS.md).
|
||||
|
||||
## Notes
|
||||
|
||||
- Probe tests were run as `tests/zzz_probe*.rs` in-tree during the
|
||||
session and deleted before any commit (the #006 pattern). None was a
|
||||
crash hazard; the amplification probe allocates ~477 MB transiently
|
||||
and completes in ~40 ms.
|
||||
- Benches are not run in CI and are excluded from the publish (the
|
||||
`[bench]` target ships — that is fine; benches don't affect the
|
||||
library's API or its wasm compatibility).
|
||||
- The 0.3.0 publish proceeds after these fixes: 0.1.0 and 0.2.0 are
|
||||
on crates.io; this is the first 0.3.0 publish, so F1/F2's
|
||||
behavior changes (both "previously-divergent, now-agreed" shapes)
|
||||
land inside the version's first release — no semver bump implied.
|
||||
-18
@@ -344,24 +344,6 @@ pub struct BastField {
|
||||
}
|
||||
|
||||
impl BastField {
|
||||
/// Construct a synthetic field wrapping `ty` with no annotations.
|
||||
/// Used internally by the materializer for array elements, record
|
||||
/// values, and union variants — these are TypeRefs, not fields, so
|
||||
/// they don't carry field-level annotations. `source` is the
|
||||
/// `BastType`'s source node (or `&Value::Null` for primitives).
|
||||
#[allow(dead_code)]
|
||||
pub(crate) fn synthetic(ty: BastType, source: Value) -> Self {
|
||||
Self {
|
||||
name: String::new(),
|
||||
ty,
|
||||
endian: None,
|
||||
align: None,
|
||||
encoding: VariableEncoding::LengthPrefixed,
|
||||
max_length: None,
|
||||
source,
|
||||
}
|
||||
}
|
||||
|
||||
/// The field name. Guaranteed to match `^[a-zA-Z_][a-zA-Z0-9_]*$` by
|
||||
/// the meta-schema; the parser does not re-check the pattern.
|
||||
pub fn name(&self) -> &str {
|
||||
|
||||
+34
-2
@@ -244,7 +244,7 @@ fn materialize_plan_array(
|
||||
)))
|
||||
}
|
||||
};
|
||||
let mut arr = Vec::with_capacity(count);
|
||||
let mut arr = Vec::new();
|
||||
for i in 0..count {
|
||||
let path = format!("{field_path}[{i}]");
|
||||
let before = *offset;
|
||||
@@ -647,7 +647,7 @@ fn materialize_array_packed(
|
||||
) -> Result<Value, AlkTypeError> {
|
||||
let element_ty = array.element();
|
||||
let count = array.count();
|
||||
let mut arr = Vec::with_capacity(count);
|
||||
let mut arr = Vec::new();
|
||||
for i in 0..count {
|
||||
let path = format!("{field_path}[{i}]");
|
||||
let resolved_elem = doc.resolve_typeref(element_ty)?;
|
||||
@@ -2208,4 +2208,36 @@ mod tests {
|
||||
let value = materialize_packed_strict(&root, &doc, &[1, 2, 3]).expect("materialize");
|
||||
assert_eq!(value, json!({ "items": [ { "v": 1 }, { "v": 2 }, { "v": 3 } ] }));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn r8_deeply_nested_stride0_array_rejects_before_bulk_prealloc() {
|
||||
// The perf commit reintroduced Vec::with_capacity(count) on both
|
||||
// array walkers. With MAX_ARRAY_ELEMENTS (2^16) the per-array
|
||||
// cap bounds each request, but nesting compounds: ~100 legal
|
||||
// levels × count 65535 held live across each level's element
|
||||
// walk measured ~477 MB of simultaneous allocation from a ~1 KB
|
||||
// schema and an empty buffer. H1's layer-1 rule (no count-sized
|
||||
// prealloc on untrusted input) is restored: Vec::new() + push.
|
||||
// This test pins the *rejection* half (the walk still errors
|
||||
// cleanly at the innermost level); the allocation shape cannot
|
||||
// be asserted cheaply in-tree, but any regression to
|
||||
// with_capacity would be caught by re-running the #008 probe.
|
||||
let mut kind = json!({ "kind": "struct", "fields": [] });
|
||||
for _ in 0..100 {
|
||||
kind = json!({ "kind": "array", "element": kind, "count": 65535 });
|
||||
}
|
||||
let root = json!({
|
||||
"$defs": {
|
||||
"S": {
|
||||
"kind": "struct",
|
||||
"endian": "little",
|
||||
"fields": [{ "name": "arr", "kind": kind }]
|
||||
}
|
||||
}
|
||||
});
|
||||
let doc = doc_from(&root, "S");
|
||||
let err = materialize_packed_strict(&root, &doc, &[]).unwrap_err();
|
||||
assert!(matches!(err, AlkTypeError::Access { .. }), "got {err:?}");
|
||||
assert!(err.to_string().contains("consumed 0 bytes"), "got {err:?}");
|
||||
}
|
||||
}
|
||||
+60
-2
@@ -527,6 +527,14 @@ fn compile_union(
|
||||
/// bytes, so such a table is simply not built (the string path also
|
||||
/// handles it correctly — it would never match, erroring at read time
|
||||
/// exactly as before).
|
||||
///
|
||||
/// Only canonical decimal keys (`i.to_string() == key` for some `u64 i`)
|
||||
/// build the table. A non-canonical key such as `"01"` or `"+1"` parses
|
||||
/// as an integer but stringifies differently — under the numeric table
|
||||
/// the reader would dispatch on it while the materializer, validation
|
||||
/// plan, and tunion (all of which stringify) would reject the same
|
||||
/// buffer. `None` keeps every consumer on the same string compare,
|
||||
/// matching the pre-int_keys behavior for such keys.
|
||||
fn compile_int_keys(
|
||||
variants: &[(String, CompositePlan)],
|
||||
_path: &str,
|
||||
@@ -534,8 +542,8 @@ fn compile_int_keys(
|
||||
let mut out = Vec::with_capacity(variants.len());
|
||||
for (i, (key, _)) in variants.iter().enumerate() {
|
||||
match key.parse::<u64>() {
|
||||
Ok(v) => out.push((v, i)),
|
||||
Err(_) => return Ok(None),
|
||||
Ok(v) if v.to_string() == *key => out.push((v, i)),
|
||||
_ => return Ok(None),
|
||||
}
|
||||
}
|
||||
Ok(Some(out))
|
||||
@@ -1338,6 +1346,56 @@ mod tests {
|
||||
assert!(matches!(err, AlkTypeError::Schema(_)), "got {err:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn r8_non_canonical_mapping_key_disables_int_dispatch() {
|
||||
// "01" parses as u64 1 but stringifies differently. If the int
|
||||
// table were built, the reader would dispatch disc 1 to the "01"
|
||||
// variant while the materializer (stringify "1"), validation plan,
|
||||
// and tunion reject it — a cross-consumer divergence. The table
|
||||
// must be disabled so every consumer string-compares.
|
||||
let root = json!({ "$defs": {
|
||||
"U": {
|
||||
"kind": "union",
|
||||
"discriminator": { "kind": "byte", "offset": 0, "type": "uint8" },
|
||||
"mapping": {
|
||||
"01": { "kind": "struct", "fields": [ { "name": "a", "kind": "uint8" } ] },
|
||||
"2": { "kind": "struct", "fields": [] }
|
||||
}
|
||||
},
|
||||
"S": { "kind": "struct", "fields": [
|
||||
{ "name": "msg", "kind": { "$ref": "#/$defs/U" } }
|
||||
]}
|
||||
}});
|
||||
let plan = plan(&root, "S");
|
||||
let Some(CompositePlan::Union { int_keys, .. }) = plan.fields()[0].body() else {
|
||||
panic!("expected a union composite");
|
||||
};
|
||||
assert!(int_keys.is_none(), "non-canonical key must disable the int table");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn r8_canonical_mapping_keys_keep_int_dispatch() {
|
||||
let root = json!({ "$defs": {
|
||||
"U": {
|
||||
"kind": "union",
|
||||
"discriminator": { "kind": "byte", "offset": 0, "type": "uint8" },
|
||||
"mapping": {
|
||||
"1": { "kind": "struct", "fields": [ { "name": "a", "kind": "uint8" } ] },
|
||||
"2": { "kind": "struct", "fields": [] }
|
||||
}
|
||||
},
|
||||
"S": { "kind": "struct", "fields": [
|
||||
{ "name": "msg", "kind": { "$ref": "#/$defs/U" } }
|
||||
]}
|
||||
}});
|
||||
let plan = plan(&root, "S");
|
||||
let Some(CompositePlan::Union { int_keys, .. }) = plan.fields()[0].body() else {
|
||||
panic!("expected a union composite");
|
||||
};
|
||||
let table = int_keys.as_ref().expect("canonical keys keep the int table");
|
||||
assert_eq!(table, &vec![(1u64, 0usize), (2, 1)]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn non_struct_root_is_schema_error() {
|
||||
let root = json!({ "$defs": {
|
||||
|
||||
Reference in new issue
Block a user