fix: embed metadata in piped stream fragment #1 by reducing payload#3457
Closed
stephensong wants to merge 2 commits intofreenet:mainfrom
Closed
fix: embed metadata in piped stream fragment #1 by reducing payload#3457stephensong wants to merge 2 commits intofreenet:mainfrom
stephensong wants to merge 2 commits intofreenet:mainfrom
Conversation
Contributor
Claude Rule Review: Failed to runThe Claude rule review step failed (likely an authentication or API issue). Check the workflow logs for details. Advisory review against |
stephensong
added a commit
to stephensong/freenet-core
that referenced
this pull request
Mar 7, 2026
Address review findings on PR freenet#3457: - Replace .unwrap() calls in tracing::debug! with local bindings - Replace debug_assert! with actual leftover fragment send for degenerate single-chunk streams, preventing silent data loss Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
|
@iduartgomez — this PR and its companion #3455 are non-trivial changes to piped stream metadata embedding and overflow fragment handling in the transport layer. They've been open for a week with no review. Could you take a look at whether this "leftover bytes propagation" approach is the right design? The referenced issue #3451 is already closed, so it's also worth confirming whether these fixes are still needed. [AI-assisted - Claude] |
…yload pipe_stream() never embedded metadata in fragment freenet#1 because the inbound payload (from upstream's send_stream()) plus metadata always exceeded MAX_DATA_SIZE. The check used packet_data::MAX_DATA_SIZE (1463) instead of the module-level MAX_DATA_SIZE (1422, which accounts for the 41-byte StreamFragment serialization overhead). Even with the correct constant, the inbound fragment freenet#1 payload may still be too large when the pipe's metadata differs in size from the upstream's. The fix reduces fragment freenet#1 payload to make room for metadata (matching send_stream's approach) and carries the remainder as leftover bytes that get prepended to the next fragment. This leftover propagates through subsequent fragments until the last one absorbs it, maintaining the same fragment count and avoiding cascading growth across multi-hop pipes. Without this fix, piped streams rely solely on the standalone UDP metadata message. When that message is lost or delayed, the receiving node cannot process the RequestStreaming, causing the entire PUT to timeout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review findings on PR freenet#3457: - Replace .unwrap() calls in tracing::debug! with local bindings - Replace debug_assert! with actual leftover fragment send for degenerate single-chunk streams, preventing silent data loss Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
stephensong
force-pushed
the
fix/pipe-stream-metadata-embedding
branch
from
7c261dd to
b7babfa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pipe_stream()never embedded metadata in fragment Overall architecture RFC #1 because the size check usedpacket_data::MAX_DATA_SIZE(1463) instead of the module-levelMAX_DATA_SIZE(1422, which accounts for the 41-byteStreamFragmentserialization overhead). This made every piped stream rely solely on the standalone UDP metadata message — when lost, the PUT times out.send_stream()'s approach). Leftover bytes are prepended to the next fragment and propagate through the chain until the last fragment absorbs them, preserving the fragment count across multi-hop pipes.Details
The metadata embedding feature (fix #2757) provides a reliability backup: metadata is embedded in fragment #1 so that if the standalone UDP metadata message is lost, the receiver can still process the stream. However, in
pipe_stream(), the inbound fragment #1 payload (already reduced by the upstream sender) plus the pipe's metadata exceeds the maximum. The old code simply skipped embedding.The fix:
MAX_DATA_SIZEconstant (1422 vs 1463) for the availability checkMAX_DATA_SIZE, truncates again and carries forwardMAX_DATA_SIZE) absorbs the final leftoverTest plan
cargo check -p freenet— compiles clean🤖 Generated with Claude Code