Negotiate window scaling so a peer's window reaches past 65535

[IntellyCode]

Problem. The TCP header's window field is 16 bits, so a receiver can advertise at most 65535 bytes. A sender may have at most one window of data outstanding before it must wait a round trip for an acknowledgement, so throughput is bounded by window ÷ RTT. At 65535 bytes and a 200 ms RTT that is 327 kB/s, about 2.6 Mbit/s, whatever the link can carry.

Solution. RFC 7323 §2 carries a 30-bit window in the 16-bit field by agreeing a power-of-two scale factor during the handshake: the real window is field << S, with S capped at 14 so the maximum stays under 2^30. The option is exchanged on the SYN only (§2.2), both sides must send it to enable either direction (§2.2), the window field of a SYN is itself unscaled (§2.2), and thereafter SND.WND = SEG.WND << Snd.Wind.Shift and SEG.WND = RCV.WND >> Rcv.Wind.Shift (§2.3). S is derived from the receive buffer (§2.1). At S=7 the ceiling becomes 8,388,480 bytes.

Also: the initial send window is the opening SYN's window, per RFC 9293 §3.10.7.4. And RFC 5681 §2's duplicate-acknowledgement test compares both windows through the same shift.

Tests. Eight. Five in tcb.rs: shift taken from the SYN, no option leaving windows unshifted, a shift above 14 clamped, a zero peer window honoured, and the advertised window derived from the read buffer including the rounding a 1.2 MiB buffer forces. Three in tcp.rs driving real handshakes: the scaled case, the clamped case, and a closed peer window holding a write until a later segment reopens it.

[Copilot]

🟡 Changes recommended

Reopen write waiters when a zero window becomes nonzero, including during the handshake, to prevent writes from remaining pending indefinitely.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds RFC 7323 TCP window-scale negotiation, enabling TCP windows larger than 65,535 bytes.

Changes:

• Negotiates and clamps window-scale shifts during SYN exchange.
• Applies scaling to send and receive window calculations.
• Adds unit and handshake tests for scaling and zero-window behavior.

File summaries

File	Description
src/stream/tcp.rs	Negotiates options, applies scaled windows, and adds integration tests.
src/stream/tcb.rs	Stores scale factors, performs shifted calculations, and adds unit tests.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Code Review: origin/window-scaling

Branch: origin/window-scaling (2 commits on top of main)
Files changed: tcb.rs, tcp.rs
Tests: ✅ All 21 unit tests + 23 doc-tests pass

---

Summary

Two tightly coupled features stacked in two commits:

1. Commit e1d8506 — Enforce the TCP receive window to bound per-session memory
2. Commit 55025b2 — Negotiate window scaling so a peer's window reaches past 65535

Together they add RFC 7323 window-scaling negotiation, enforce receive-window backpressure, and convert the handoff channel from unbounded to bounded.

---

Architecture & Design

What's being done

Area	Before	After
Peer's advertised window	Stored as raw u16, hard-capped at 65 535	Stored as u32, shifted left by the peer's negotiated scale factor
Own advertised window	get_recv_window() returning unscaled free buffer	New get_scaled_recv_window() shifts right by own scale; SYN window left unscaled per RFC
Data handoff channel	UnboundedSender<Vec<u8>> — no backpressure	Bounded Sender<Vec<u8>> with capacity read_buffer_size / READ_CHUNK
Backpressure	None — a slow reader could let memory grow without limit	try_reserve() before consuming; full channel → ACK with current window → peer slows or enters persist
Receive buffer enforcement	None	Out-of-order segments beyond read_buffer_size are dropped (head-of-line always admitted)
Overlap trimming	Not handled	consume_unordered_packets trims stale head entries left by re-segmented retransmissions
FIN acceptance	Unconditional	FIN+ACK accepted only when tcb.get_ack() == incoming_seq (all prior data consumed)
Options plumbing	create_raw_packet received Option<&Vec<TcpOptions>>, converted internally	Caller builds Vec<TcpOptionElement> directly; create_raw_packet takes &[TcpOptionElement]

Tip

The design follows RFC 7323 §2.2–2.3 faithfully: window scale travels only on SYN, SYN windows are unscaled, the shift is clamped to 14, and both directions negotiate independently.

---

Strengths

1. Correct RFC 7323 semantics — The SYN/SYN-ACK exchange is the only place where window scale is read/written. The SYN's own window is correctly left unscaled. Shift counts above 14 are clamped with a warning.

2. Memory safety via backpressure — Converting the unbounded channel to bounded + try_reserve() before consuming is a clean pattern: data only leaves the reassembly map once it has a guaranteed slot, so ack always reflects what the reader can actually absorb.

3. Silly-window-syndrome avoidance — Advertising zero when the free buffer is below one MSS is a good practical measure that will put the peer into persist mode rather than having it push tiny segments.

4. Overlap/retransmission handling — The new seq < self.ack branch in consume_unordered_packets handles a real edge case where a retransmission is re-segmented across the current ack boundary.

5. Excellent test coverage — 10+ new tests covering: window scaling negotiation and honouring, clamping, zero-window, receive buffer enforcement, overlap trimming, and the reserve-before-consume flow. The integration tests in tcp::tests exercise the full IpStackTcpStream → SYN-ACK → data path.

6. Clean refactoring of options plumbing — Moving from Option<&Vec<TcpOptions>> → &[TcpOptionElement] eliminates an indirection layer and the internal conversion loop.

---

Issues & Suggestions

🔴 Potential bugs

1. send_window initialized from unscaled SYN window

// tcb.rs constructor:
send_window: peer_window as u32,   // ← raw u16 from the SYN

This is correct per RFC 7323 §2.3 ("The window field in a SYN segment itself is never scaled"). But honoured_window() also has the SYN guard:

if tcp_header.syn { window } else { window << self.send_window_shift }

So when update_send_window is called with the incoming SYN header (which happens at the bottom of the main loop), it would re-set send_window to the unscaled value — which is fine. No bug, but worth a comment in the constructor explaining this mirrors the RFC's "SYN window is never scaled" rule so reviewers don't double-take.

2. drain_notify wake without holding the lock

In AsyncRead::poll_read, after consuming from data_rx:

self.drain_notify.notify_one();

This races with the main loop's drain_notify.notified() branch. Since tokio::sync::Notify::notify_one() stores a permit if no one is currently waiting, the notification won't be lost. No bug — but this is subtle and worth a comment noting the permit-based wake guarantee.

3. FIN guard uses == which may be too strict

} else if flags == (ACK | FIN) && tcb.get_ack() == incoming_seq {

If a FIN arrives with data still buffered in the reassembly map (but the channel is full), the FIN is ignored and the peer must retransmit. This is intentional per the commit message ("a FIN is accepted only once the data before it has been consumed"), but if the peer retransmits the FIN with a different sequence number due to re-segmentation, it could be permanently rejected. Low risk — real stacks rarely re-segment FINs — but consider logging when a FIN is rejected so it's diagnosable.

🟡 Suggestions

4. get_unordered_packets_total_len() is O(n)

pub(crate) fn get_unordered_packets_total_len(&self) -> usize {
    self.unordered_packets.values().map(|p| p.len()).sum()
}

This is called on every incoming segment (in add_unordered_packet), in get_available_read_buffer_size (called for every advertised window), and in extract_data_n_write_upstream. Consider tracking the total length in a field, incrementing on insert and decrementing on remove, to make it O(1).

5. Tcb::new has 9 positional parameters

The #[allow(clippy::too_many_arguments)] suppresses the lint, but with two new parameters (peer_window, peer_window_shift) this is getting unwieldy. A builder pattern or a TcbConfig struct would make call sites clearer and less error-prone, especially since several parameters are bare numeric types where transposition is easy to miss.

6. recv_window_shift as None vs Some(0) semantics

recv_window_shift is None when the peer didn't offer window scaling, and Some(0) when the peer offered it but the stack's buffer fits in 16 bits. This is correct, but the dual meaning of Option (feature-not-negotiated vs negotiated-at-zero) could be made more explicit with a comment or a small enum. Currently get_scaled_recv_window() uses unwrap_or(0) which accidentally works for both cases but hides the distinction.

7. Bounded channel capacity could be documented

let data_channel_len = config.read_buffer_size.div_ceil(READ_CHUNK).max(1);

This means the default 16 KiB buffer / 8 KiB chunk = 2 slots. The relationship between channel depth, backpressure aggressiveness, and the advertised window could be documented in a comment or in the TcpConfig docs.

8. Consider clippy::let_and_return in check_pkt_type

The rcvd_window comparison on line 292:

} else if self.get_send_window() == rcvd_window && ...

Now compares u32 == u32 where previously it was u16 == u16. This is correct after the type change, just confirming the comparison semantics didn't silently change.

🟢 Nits

• Line 292 (tcb.rs): The get_send_window() == rcvd_window duplicate-ack detection now compares scaled windows. This is correct — if both sides agree on scaling, the comparison still identifies duplicates.

• window_tcb test helper (tcb.rs): Nice factoring. Consider adding it to a test_utils module if more test files need it.

• open / feed / window_scale_of test helpers (tcp.rs): Well-designed test infrastructure. The open helper constructing a full IpStackTcpStream from a raw SYN is particularly useful for future tests.

---

Verdict

Approve with minor suggestions. The implementation is solid, follows the RFC correctly, has excellent test coverage, and solves a real memory-safety concern (unbounded buffering). The suggestions above are all improvements rather than blockers. The most impactful one to consider addressing before merge is #4 (O(n) get_unordered_packets_total_len) since it's on the hot path and trivial to fix with a running counter.

[IntellyCode]

Inline comment (tcp.rs, waking a writer on a zero window): correct; fixed in 97f856b by waking the writer in the SynReceived and ACK|PSH arms, covered by zero_peer_window_holds_the_writer_until_it_reopens.

[SajjadPourali]

All good, thanks.