LAB-375: fix Set-branch aggregate byte-budget and count-cap fail-fast in interop encodeCanonical#72
LAB-375: fix Set-branch aggregate byte-budget and count-cap fail-fast in interop encodeCanonical#7227Bslash6 wants to merge 1 commit into
Conversation
…encode The Set branch of encodeCanonical seeded every element sub-sink from the frozen parent total, so a Set whose elements are individually small but collectively over budget encoded and buffered all N elements before ValueTooLargeError fired — peak transient memory ~sum(element bytes) instead of ~1 MiB, contradicting ChunkSink's rejected-during-traversal contract (the residual case of cd80805). Thread a running byte total across element sub-encodes and dedupe on insert (keyed by encoded bytes) so both the byte budget and the collection-size cap fire mid-iteration while counting exactly what the output retains: duplicate-heavy Sets whose deduped encoding fits still encode byte-identically instead of being rejected at their pre-dedupe sum, and the count cap bounds retained buffers at 10,000 elements. Canonical byte output is unchanged for every accepted input — all 110 protocol vectors pass byte-for-byte. LAB-375 Co-authored-by: multica-agent <github@multica.ai>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 41 minutes. |
|
Mandatory crypto/protocol expert panel (critical stakes) is clean — unanimous SHIP, zero findings. Byte-identical wire output was proven (old sort-then-adjacent-dedup ≡ new dedup-on-insert+sort; the new accepted-set is a strict subset of the old, so no cross-SDK decode risk — it only rejects more, always |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Closes LAB-375 — the residual finding from the #71 review cycle (#71 merged before this could land on its branch, so it ships as a fast-follow).
Problem
The Set branch of
encodeCanonicalseeded every element sub-sink from the frozen parent total and only advanced the parent budget in the final push loop. A Set whose elements are each under the 1 MiB budget but collectively over it fully encoded and buffered all N elements beforeValueTooLargeErrorfired — peak transient memory ≈ Σ(element bytes), contradictingChunkSink's own "rejected during traversal" contract. The element-count cap (10,000) had the same timing: it fired inencodeArrayHeaderafter the loop, so a huge-count Set retained every per-element buffer first.Fix
Thread a running byte total across element sub-encodes and dedupe on insert (keyed by the element's encoded bytes via the already-imported
bytesToHex):checkCollectionSize(seen.size)—seen.sizeis by construction the post-dedupe count, so acceptance is identical to the post-loop header check, just earlier. Retained buffers are now bounded (~1 MiB payload / 10,000 elements) for every input.One documented conservative edge remains, inherent to fail-fast: a duplicate element arriving with less than its own encoded size left in the byte budget throws mid-encode (duplicateness is unknowable until encoded). That only fires when the deduped total is already within one element of the cap.
Byte-identity (load-bearing)
The running total and seen-set feed only the failure checks, never the emitted chunks. All 110 protocol tests pass byte-for-byte, including the Set-ordering vectors (mixed-sign
{-5,"a",1.5}→["a",1.5,-5]) and the{2,2.0}dedupe vector. Full suite 559 passed, lint/prettier/type-check clean.Tests
Three regression tests (getter-spy elements count how many Set members actually encoded, pinning the timing, which a plain
toThrowcannot — the old code also threw, just after buffering everything):Review gate
Mandatory crypto/protocol expert-panel review (high stakes) ran before this PR: bug-hunter, security, craftsman, and pragmatism panelists. All surviving findings are applied in this diff (dedupe-on-insert acceptance fix, in-loop count cap, comment/doc corrections, test placement). One out-of-scope finding —
encodeMapEntriesmaterialises and sorts all map keys before its count cap fires (pre-existing, same class) — is raised as a separate ticket rather than expanding this diff.