descriptor: preserve existing message on decode errors#299
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
The fix changes observable decode behavior, so it belongs in the release notes. No-Verification-Needed: changelog fragment only
iainmcgin
left a comment
There was a problem hiding this comment.
[claude code] Thanks — this is a real owned/reflective parity gap and the fix is the right shape. Reviewed the control flow closely: ctx.descend()? runs before the remove, every ? and early return Err now lives inside the closure, and self.fields.insert(...) always runs before result propagates, so no non-panic exit can drop the value. Restoring the partially-merged value (rather than a pre-merge snapshot) is also correct — protobuf merge accumulates field-by-field with no rollback, and that is what the owned decoder leaves behind on error, so this closes the divergence rather than trading it for a new one.
One correction to the PR description for the record: there is no clone. The description says "existing singular message values are cloned before wire merging", but the code moves the value out with remove and moves it back with insert. That's the better design — a take-and-restore rather than a defensive copy — and worth stating accurately so nobody goes looking for a hot-path clone that isn't there.
I pushed one commit (7e388ff) adding the missing changelog fragment. The fix changes observable decode behavior, so it belongs in the 0.9.0 notes; check-changelog only verifies CHANGELOG.md is in sync with the fragments, so its passing here wasn't evidence a fragment wasn't needed.
On the test coverage — not blocking, but worth knowing for the sibling PRs. I measured which of the new tests actually guard the regression by reverting dynamic.rs to this branch's merge-base and re-running them:
| test | vs merge-base |
|---|---|
..._when_length_varint_is_truncated |
fails — guards the fix |
..._when_declared_length_is_too_large |
fails — guards the fix |
..._when_nested_payload_is_malformed |
passes |
..._when_wrong_wire_type_is_truncated |
passes |
..._when_group_payload_is_malformed |
passes |
Two of the five are load-bearing. The other three exercise arms where the merge call is the tail expression (merge_buf, merge_group), which flowed through to the reinsert even before this change — and the wrong-wire-type case is rejected by the wire_type_compatible guard upstream, so it never reaches merge_into_existing_message at all. They aren't wrong and I've left them: they document the intended behavior and will catch a future edit that introduces a ? after those tails, which is exactly the shape of the bug you fixed. Just don't read them as five guards where there are two. (The ..._does_not_fit_usize case is #[cfg(target_pointer_width = "32")], so it never runs on CI's 64-bit runners.)
Also confirmed the sibling merge paths are clean: merge_list_field and merge_map_field both use entry().or_insert_with() and mutate in place, so they have no remove-then-fail hazard. Oneof is #303's territory — no overlap with this diff.
Approving — thank you for the fix and for the thorough error-path tests.
What this does
Preserves an existing singular message value when a later wire occurrence fails during decoding.
merge_into_existing_messagetemporarily removes the existing value while merging the new occurrence. Decode errors could return before reinserting it, silently dropping data already decoded. The merge result now runs inside a closure so the value is restored before the error is returned.Coverage
The regression tests cover truncated length varints, declared lengths larger than the remaining input, platform-specific length overflow, malformed nested messages, malformed wrong-wire input, and malformed group occurrences.
Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspacecargo test -p buffa-descriptor --features reflectcargo check --workspace --all-features