Data-plane follow-ups: ARQ integrity test, detector completeness, retx sizing, hardening - #4
Merged
Merged
Conversation
… duplicate put 1. Fix doc comment on AdaptiveController::new: state it delegates to new_for and thus has class-aware behavior (kept for API compatibility only). 2. Fix comment in tunnel.rs near fraction_f32: accurately describe the u16→f32 conversion path that the code actually uses (no f64 intermediate). 3. Harden RetxBuffer::put against duplicate counters: if a counter already exists, update it in place without pushing to the order deque (which would create a phantom entry). Add unit test retx_duplicate_put_no_phantom_entry to verify len stays 1 after duplicate puts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
At ~100k objects/s the 1024-entry cap held sent objects only ~10ms, shorter than the feedback interval + RTT, so NACKed objects were evicted before retransmit. 16384 covers ~160ms; TTL still bounds memory at lower rates (~20MiB worst case).
…ets delivered Extend LossDetector from a gap-only model to a unified pending model: on_seen(c) now also adds the received counter itself to the pending-awaiting-delivery set (not just implied-gap counters), so multi-symbol FEC objects that received ≥1 symbol but never decoded are reported missing after grace_ms. Resolved-counter tracking prevents the re-add edge: a monotone watermark (resolved_below) plus a bounded out-of-order resolved_set (capped at window entries) let on_seen ignore any counter that is already delivered-or-reported; late duplicate symbols for a completed object are silently dropped. Repeated on_seen for the same in-flight counter is idempotent (first-seen timestamp preserved). In tunnel.rs, the ingress control-packet handler now calls on_delivered immediately after on_seen for every authenticated control packet, so unified-pending does not falsely report control counters as lost data objects. New tests: seen_but_undelivered_reported_after_grace, late_duplicate_after_delivery_ not_reported, delivered_within_grace_not_reported, repeated_on_seen_is_idempotent, already_reported_missing_not_re_reported. All 53 yip-transport tests pass; both netns tunnel tests pass (clean and 10% loss). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r + silent-link cadence - lossdetect: resolved_below advance uses saturating_add (panic-free at u64::MAX). - tunnel.rs: document that the per-class loss denominator (sent-log residency) biases the fraction high => conservative over-repair (never under-protects); an exact per-class windowed rate is intractable here and not worth the under-repair risk. Document that batch-gated feedback on a silent inbound link is correct (no received data => no loss to report).
Adds a shared Arc<AtomicU64> retransmit counter in tunnel.rs incremented once per Bulk object retransmitted via NACK; exposed in the 5 s periodic log as "yipd [Xms] ARQ retransmits: N". Adds run-arq-integrity.sh and the arq_recovers_bulk_loss integration test: two yipd in netns, handshake on a clean link, then apply tc netem 5% loss + 5ms delay; blasts 20 000 × 1 400-byte UDP packets (Bulk class, arq=true) B→A and asserts (a) delivery ≥ 98% and (b) arqB log shows ARQ retransmits > 0, proving ARQ fired distinct from proactive FEC. Measured: 99.2–99.3% delivery, 133–154 retransmits. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
12 tasks
vxfemboy
added a commit
that referenced
this pull request
Jul 26, 2026
perf(yip-bench): CPU-bound-regime spike (#4 go/no-go)
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
1 participant
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.
Addresses all follow-ups logged during the loss-feedback + ARQ milestone reviews. Each change individually reviewed; the end-to-end ARQ test (FU-A) passing on the full branch is the cross-cutting integration proof.
Real improvements
AtomicU64ARQ-retransmit counter + periodic log for observability.RETX_BUFFER_MAX1024 → 16384. At ~100k obj/s the old cap held only ~10 ms (< feedback RTT), so NACKed objects were evicted before retransmit; 16384 covers ~160 ms (~20 MiB worst case).resolved_belowwatermark +resolved_setthat suppresses late-duplicate-after-delivery re-reports. Control packets are marked delivered after auth so they aren't false-reported.Hardening + documentation
resolved_belowadvance usessaturating_add(panic-free at u64::MAX); documented the per-class loss denominator as an accepted conservative approximation (biases toward more repair under loss — never under-protects) and the batch-gated feedback cadence as a non-issue (a silent inbound link has no loss to report).AdaptiveController::newdoc + atunnel.rscomment drift; guardedRetxBuffer::putagainst duplicate counters (with a test).Coverage 90.54%; all netns tests pass (ping 3/3, ping-under-loss, arq-integrity 145 retransmits).
🤖 Generated with Claude Code