Conversation
CI status, and the gate's scatter on the small benchmarks11 of 11 registered checks pass. The four that did not register are CodeQL's ( The benchmark gate failed once and passed on re-run, on the same five benchmarks that did this on #258. There are now five measurements of this code against master, on four different CPU models:
Two things to read off it. These four benchmarks carry roughly ±10% run-to-run scatter that the build does not explain. What is consistent is the thing that is real. For this PR's own effect, the comparison that isolates it is the local interleaved A/B against its actual base in the PR body: worst real movement −2.0% ( |
MimePart::build (full mode, #99) and build_node (the deferred modes, #202) were the same recursive walk with a different leaf arm. Both checked the depth cap, both read ctype.mimetype and the disposition, both recursed over subparts for multipart/*, both carried a verbatim copy of the message/rfc822 decode -> repair_missing_separator -> parse_mail -> recurse block, and both ended in the same six-field node literal. The only thing that differed was what a leaf's body became. So every per-node rule the tree enforces -- the depth cap, the embedded-message repair, how Content-ID is normalised, how the disposition token is derived -- had to be edited in two places, and the project paid for a fuzz invariant whose only job was to notice when the copies drifted. One of those copies was of the depth cap and of the re-parse of an attacker-supplied embedded message, which is not a block to maintain two of. The node is now generic over its body: pub struct Node<B> { ... body: B, children: Vec<Node<B>> } pub type MimePart = Node<Option<Vec<u8>>>; pub type TreeNode = Node<NodeBody>; and one traversal is driven by a LeafPolicy, which says what a body is and nothing else. There are two implementations, so the linker sees the two instantiations it saw before: Full, and the Retain enum #239 already used to say what a leaf keeps of itself -- Retain IS the deferred leaf policy, which is why these two issues wanted doing together. NodeBody stays, and so does the reason for it: MimePart's None *means* container, and a mode where None could also mean "not decoded yet" would make the two indistinguishable. Generic over the body satisfies that without a second walk. grep -c 'if depth >= MAX_MIME_DEPTH' in the core goes 3 -> 2 (the traversal and the flat extract_mail_parts). The embedded-message block appears once. The inline(never) comment now describes the function it is attached to, which it did not before. Nothing public moves. MimePart.content is renamed to .body inside the core; the binding reads it at one line and the fuzz target at three. __init__.pyi, tests/test_contract.py, docs/ and vendor/ are untouched. Measured flat, which is the claim a cold-code change has to make here: local interleaved A/B on an M4, 3 rounds, worst real movement -2.0% against a 1.8% control noise floor. 864 Python tests, 11 core tests (one new: the always-run twin of fuzz invariant 8, asserting the three modes build identical non-body node fields around a nested message/rfc822), 338K fuzz executions with debug assertions live, clippy, fmt, mypy --strict, ruff, the vendored mailparse suite and both repo invariant scripts.
80b4da6 to
9613d16
Compare
1 participant
Closes #237.
The duplication
MimePart::build(full mode, #99) andbuild_node(the deferred modes, #202) were the same recursive walk with a different leaf arm — same depth cap, samemultipart/*recursion, a verbatim copy of themessage/rfc822decode → repair → re-parse block, and the same six-field node literal. Every per-node rule had to be edited twice, and a fuzz invariant existed to notice when the copies drifted.One of those copies was of the depth cap and the re-parse of an attacker-supplied embedded message. That is the part worth not having two of.
The shape
Two implementations, so the linker sees the two instantiations it saw before:
Full, andRetainfrom #239.inside_embeddedis a hook rather than a constant precisely because of #239 — a mode that retains offsets has to stop retaining them inside an embedded message, whose bytes are a decode of the enclosing body.NodeBodystays, and so does its reason:MimePart'sNonemeans container, and a mode whereNonecould also mean "not decoded yet" would make the two indistinguishable. Generic over the body satisfies that without a second walk.Acceptance
grep -c 'if depth >= MAX_MIME_DEPTH'in the coreextract_mail_parts)message/rfc822decode→repair→re-parse blockMimePart/TreeNodenames,parse_email_tree(payload),parse_tree_deferred(payload, bool)src/fast_mail_parser.rspart.content→part.body)__init__.pyi,tests/test_contract.py,docs/,vendor/¹
parse_tree_deferredreturnsDeferredTreerather thanTreeNode, which is #258's change, not this one.The false comment is fixed too:
build_nodeclaimed to be#[inline(never)]"likeMimePart::build", which had no such attribute.Measurements
Local interleaved A/B on an M4 (
Apple M4, 10 vCPU), 3 rounds, against this PR's base:parse_treeparse_tree_metadataparse_tree_lazy_untouchedparse_messageparse_metadataparse_lazy_untouchedparse_manyNoise floor from the pure-Python controls: 1.8%. Worst real movement −2.0% (
parse_tree), whose per-round values are 0.156 / 0.156 / 0.155 against 0.156 / 0.159 / 0.162 — identical in round 1 and drifting upward after, which is thermal, not codegen. Flat, as a maintenance change should be; the x86 gate is the verdict.ab_median.pyalso flagsattachment_rereadat "+97.6%" in this PR's favour. Ignore it: that benchmark is 42–83 ns and both sides sampled the same two timer ticks (base 83/42/83, this PR 83/42/42). It is quantisation, not a measurement.Verification: 864 Python tests including all of
test_mime_tree.pyandtest_tree_modes.pywith noDIVERGENCESedits; 11 core Rust tests;cargo clippy --all-targets -D warnings -W clippy::cast_possible_truncationon all three crates;cargo fmt;mypy --strict;ruff; the vendored mailparse suite (82 tests, external--target-dir);check_vendored_mailparse.shandcheck_bench_lockfile.py. 338Kcargo fuzz run -a parse_agreementexecutions, zero crashes — that target's invariant 8 is the shape oracle this change has to satisfy, and it now also has an always-run twin incargo test(every_mode_builds_the_same_tree_around_an_embedded_message).