Skip to content

fix(view): preserve packed closed-enum unknowns#252

Open
ogarciarevett wants to merge 1 commit into
anthropics:mainfrom
ogarciarevett:fix/view-packed-closed-enum-unknowns
Open

fix(view): preserve packed closed-enum unknowns#252
ogarciarevett wants to merge 1 commit into
anthropics:mainfrom
ogarciarevett:fix/view-packed-closed-enum-unknowns

Conversation

@ogarciarevett

Copy link
Copy Markdown
Contributor

Closed proto2 enums already preserve unknown values on the owned path, but the packed repeated view path was still dropping them.

This keeps those packed unknown values by letting UnknownFieldsView hold synthetic unknown fields for cases where there is no borrowable per-element wire span.

I also added a regression test for the packed repeated view case and removed the corresponding item from the README limitations list.

Proof:

  • RED on main in a clean worktree: cargo test -p buffa-test 'tests::closed_enum::test_view_closed_enum_repeated_packed_unknown_preserved' -- --exact
  • GREEN here: cargo test -p buffa-test 'tests::closed_enum::test_view_closed_enum_repeated_packed_unknown_preserved' -- --exact
  • Runtime coverage: cargo test -p buffa 'view::tests::unknown_fields_view_to_owned_includes_owned_fields' -- --exact
  • Broader validation: cargo clippy --workspace --all-targets -- -D warnings and cargo test --workspace

@github-actions

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@iainmcgin iainmcgin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[claude code] Thanks for tackling this — the diagnosis is right (the view path dropped unrecognized packed closed-enum values that the owned path preserves), and routing them into the unknown set as individual varints exactly mirrors the owned path, including the as u64 round-trip of negative values. Three things need addressing before this can land:

  1. Stale base. The branch predates #184's rework of UnknownFieldsView — on current main it carries raw_spans + last_tail + to_owned_allowance with span coalescing, and to_owned threads a budgeted DecodeContext rather than calling decode_unknown_field with a bare recursion limit. GitHub reports the PR as conflicting (which is also why CI hasn't run — the pull_request workflow doesn't fire on conflicting PRs). Please rebase onto current main; the view.rs half of the fix needs reworking against that machinery.

  2. Unknown-field budget bypass. On current main, push_record consumes ctx.register_unknown_field() — the memory-amplification bound added in #184 (a DoS guard). The new push_owned path doesn't consume the allowance at decode time, and the owned fields are appended in to_owned outside the budget. A packed blob consisting entirely of unknown closed-enum varints is precisely the amplification shape the budget exists for (each 1-byte element becomes a ~16+ byte owned UnknownField). Synthetic unknowns need to draw from the same allowance.

  3. Wire-order parity. Storing synthetic unknowns in a separate owned store that is written/converted after all raw spans means a message with a packed-enum unknown at field N followed on the wire by a genuinely-unknown field M re-encodes as [M, N] from the view path but [N, M] from the owned path — a byte-order divergence from the parity goal that the new test asserts in the single-field case. Either interleave in wire order or document the limitation explicitly; and either way please extend the tests: assert !view.__buffa_unknown_fields.is_empty() in the round-trip test (so a regression that drops the unknowns but happens to re-encode identically can't pass), and add a mixed-unknowns parity case.

The codegen half (the closed_enum_view_unknown_route helper mirroring impl_message.rs's owned route, and keeping closed_enum_decode for map values) looks right and should survive the rebase mostly intact. Happy to re-review after the rebase.

@ogarciarevett

Copy link
Copy Markdown
Contributor Author

Closed proto2 enums already preserve unknown values on the owned path, but the packed repeated view path was still dropping them.

This keeps those packed unknown values by letting UnknownFieldsView hold synthetic unknown fields for cases where there is no borrowable per-element wire span.

I also added a regression test for the packed repeated view case and removed the corresponding item from the README limitations list.

Proof:

  • RED on main in a clean worktree: cargo test -p buffa-test 'tests::closed_enum::test_view_closed_enum_repeated_packed_unknown_preserved' -- --exact
  • GREEN here: cargo test -p buffa-test 'tests::closed_enum::test_view_closed_enum_repeated_packed_unknown_preserved' -- --exact
  • Runtime coverage: cargo test -p buffa 'view::tests::unknown_fields_view_to_owned_includes_owned_fields' -- --exact
  • Broader validation: cargo clippy --workspace --all-targets -- -D warnings and cargo test --workspace

Ohhh I dint see that PR merged - thanks - I'll let you know after rebase

@ogarciarevett ogarciarevett force-pushed the fix/view-packed-closed-enum-unknowns branch from 5c10388 to 172f045 Compare June 28, 2026 22:31
@ogarciarevett

Copy link
Copy Markdown
Contributor Author

@iainmcgin rebased this onto current main and reworked the view-side preservation against the #184 UnknownFieldsView machinery.

Changes from the review:

  • synthetic packed closed-enum unknowns now go through push_varint(..., ctx)?, so they consume the same unknown-field allowance as borrowed unknown records;
  • UnknownFieldsView keeps borrowed and synthetic wire records in one decode-order sequence, so mixed unknowns preserve owned-path byte order;
  • added coverage for mixed packed-unknown + later unknown ordering, unknown-field limit exhaustion, and explicit non-empty unknown fields in the packed regression.

Local checks:

  • cargo test -p buffa-test closed_enum
  • cargo test -p buffa 'view::tests::unknown_fields_view'
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo test --workspace

Ready for re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants