Skip to content

fix(sock): notice truncated control data on Windows receives - #7543

Merged
kixelated merged 5 commits into
mainfrom
quest/m2/cmsg-windows-truncation
Nov 2, 2026
Merged

kixelated merged 5 commits into
mainfrom
quest/m2/cmsg-windows-truncation

Conversation

@kixelated

@kixelated kixelated commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Windows receives returned early on WSAEMSGSIZE when Winsock truncated only the control data, dropping a complete datagram without the Unix path's warning. Completes quest/m2/cmsg-windows-truncation.md, following moq-dev/moq#7394.

Approach

  • Recover WSAEMSGSIZE only when MSG_CTRUNC is set and MSG_TRUNC is clear. Payload truncation and other errors still fail.
  • Keep the datagram and metadata that fit, using the existing bounded cmsg decoder, and warn once per process.
  • Remove the completed quest and its link. Merge current main without changing the Windows implementation.

Impact

  • Public API: none.
  • Wire: none.

Alternatives

Dropping control-truncated datagrams would discard complete payloads. The quest records the maintainer's decision to match Unix and retain them. The existing 128-byte buffer already fits the enabled metadata, so no buffer or API change is needed.

Choices made without asking

  • Use Winsock's returned byte count on the recovered error path. Its documentation does not explicitly guarantee that count on failure; if left at its initial zero, the QUIC receive loop skips the empty buffer.
  • No dedicated truncation unit test: this host cannot execute Windows code, and normal receives do not overflow the existing control buffer. Native platform checks cover compilation and the existing Windows socket tests; they do not force control truncation.

Verification

  • quest gate env -u BASH_ENV just ci: pass (184 seconds; 5,728 tests passed, 95 skipped). Uses the already-loaded pinned Nix dev shell; disabling the harness's Bash startup hook prevents every child Bash from re-entering Nix and expanding PATH until mbx fails with E2BIG. That base/environment problem is tracked by moq-dev/moq#7594.
  • Pinned Rust 1.98.1 with the Windows std targets: cargo clippy --locked -p moq-sock --all-targets --target x86_64-pc-windows-gnu -- -D warnings and cargo clippy --locked -p moq-sock --lib --target x86_64-pc-windows-msvc -- -D warnings pass at 298c9b9653bd74e345d8e47b4831e4893883fe8f.
  • quest check --base origin/main --branch quest/m2/cmsg-windows-truncation: 603 documents OK. git diff --check origin/main...HEAD: pass.
  • Native Platform run 41469613560 dispatched on 298c9b9653bd74e345d8e47b4831e4893883fe8f; pending at handback. Await the Windows job, including its Rust compile and tests step, before requeueing. No full Nightly was dispatched.
  • Forced native MSG_CTRUNC behavior, including Winsock's byte count on that error path, remains untested.

Review

  • Codex (quest review), gpt-6.1-sol, medium effort, session 01a19aa5-23b1-70b2-980b-8ddc97f5db0f, reviewed d32b0a43a850a1590233be4216cdea1a509c1645: no findings.
  • Final head 298c9b9653bd74e345d8e47b4831e4893883fe8f adds only a conflict-free merge of main 7168b8f9720df554bf53f39cd5729afefd4bcaa8, including the quest link-list merge driver. The guide exempts this from another review round. git diff d32b0a43a850a1590233be4216cdea1a509c1645 HEAD -- rs/moq-sock/src/udp/windows.rs is empty.
  • Earlier review fixed the early return on control-only WSAEMSGSIZE; the next round and existing online reviews found no further defects.

Follow-ups

The error-queue reader's truncation handling is already tracked in quest/m2/errqueue-truncation.md. No new follow-ups or maintainer decisions.

(Written by GPT-6.1 Sol)

kixelated and others added 3 commits November 1, 2026 03:10
WSARecvMsg reports MSG_CTRUNC in WSAMSG.dwFlags. Keep the datagram with
whatever metadata fit and warn once, matching the Unix receive paths from
#7394. The 128-byte buffer already fits what Windows sends, so this is
defensive.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Winsock fails WSARecvMsg with WSAEMSGSIZE when control data is truncated,
so the MSG_CTRUNC check never ran. Treat WSAEMSGSIZE with MSG_CTRUNC and
without MSG_TRUNC as a received datagram.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title Windows receives notice truncated control data fix(sock): notice truncated control data on Windows receives Nov 1, 2026
@kixelated
kixelated marked this pull request as ready for review November 1, 2026 11:17
@coderabbitai

coderabbitai Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 12d6e456-4ef9-4844-81e8-266e9219cef3
Commits

Reviewing files that changed from the base of the PR and between 7168b8f and 298c9b9.

Files selected for processing: 3
  • quest/m2/README.md
  • quest/m2/cmsg-windows-truncation.md
  • rs/moq-sock/src/udp/windows.rs
Files with no reviewable changes: 2
  • quest/m2/cmsg-windows-truncation.md
  • quest/m2/README.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

Walkthrough

The Windows UDP receive path now continues after WSAEMSGSIZE only when MSG_CTRUNC is set and MSG_TRUNC is not. It warns once when control data is truncated, then processes the control data that fit. Other receive errors, including payload truncation, still return the OS error. The m2 plan entry and planning note about this behavior were removed.

Merge Risk: Minimal · up to 298c9

No confirmed merge-blocking defect remains, although native Windows truncation behavior has not been verified.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title names the change: Windows receives now notice truncated control data (the MSG_CTRUNC case). The raw summary says Windows UDP receive handling treats WSAEMSGSIZE as recoverable when only cont…
Description check Passed The description is directly related to the changeset. It explains that Windows receives returned early on WSAEMSGSIZE when only control data was truncated, and it describes the fix: recovering only wh…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the change: Windows receives now notice truncated control data (the MSG_CTRUNC case). The raw summary says Windows UDP receive handling treats WSAEMSGSIZE as recoverable when only control data is truncated, logs a warning once, and keeps the datagram. The title matches that main change and is specific enough for a teammate scanning history.

Full details: Description check

Explanation

The description is directly related to the changeset. It explains that Windows receives returned early on WSAEMSGSIZE when only control data was truncated, and it describes the fix: recovering only when MSG_CTRUNC is set and MSG_TRUNC is clear, keeping the datagram, and warning once. It also covers the quest removal, the lack of public API or wire changes, and verification details.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files.

Finishing Touches
Simplify code
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit: bb538bb41121eace8caf8a899ae0fc1d1c827256.

No actionable bugs found in the changed code. The narrow WSAEMSGSIZE + control-only truncation exception is a reasonable direction: it preserves payload-truncation errors, reuses the bounded cmsg decoder, and matches the Unix once-only warning without adding another abstraction. No public API or wire changes.

The earlier issue recorded in the PR description (returning before inspecting MSG_CTRUNC) is addressed. Verification was static inspection of the diff, full Windows receive implementation, and shared/platform cmsg handling; I did not run builds or tests. Native Windows behavior, especially the returned byte count on the recovered error path, remains unverified.

(Written by OpenAI)

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Windows control truncation recovery — rs/moq-sock/src/udp/windows.rs:196-224 now treats a WSARecvMsg failure with WSAEMSGSIZE and MSG_CTRUNC set (MSG_TRUNC clear) as a recoverable partial receive: the whole payload arrived, so the datagram is kept and one warn! is emitted through a static Once, matching decode_recv in unix.rs:749-757. Any other error, payload truncation included, still returns Err.
  • Quest cleanup — deletes quest/m2/cmsg-windows-truncation.md and its quest/m2/README.md entry.

Verified the mechanism against the platform contract: WinSock::MSG_CTRUNC = 512 / MSG_TRUNC = 256 make the truncated != MSG_CTRUNC check require exactly CTRUNC-set/TRUNC-clear, the WSAMSG remarks document that truncation returns WSAEMSGSIZE with the flags in dwFlags, and the mapping of raw_os_error() to the WSA code mirrors the established is_msg_size_err in udp/mod.rs. The restored cmsg walk stays bounded by Control.len, and the residual uncertainty about lpdwNumberOfBytesRecvd on the failure path is already called out in the PR description. No public API or wire change.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

codex / Codex land fixer note:

The original batch log says at line 135, "ci: check rs cancelled in 2s", and at line 146, "ci: check python failed". This PR is not blamed for that Python failure.

cargo test --locked -p moq-sock --all-targets passed on Linux. The changed implementation is Windows-only, so I also attempted cargo test --locked -p moq-sock --all-targets --target x86_64-pc-windows-gnu. It failed with "error[E0463]: can't find crate for core" and "the x86_64-pc-windows-gnu target may not be installed". A Windows build and test run is required to verify this change. The PR branch is unchanged.

(written by codex / Codex)

@moq-bot
moq-bot Bot disabled auto-merge November 2, 2026 02:02
@moq-bot moq-bot Bot removed the quest:merge label Nov 2, 2026
@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

codex / Codex land fixer: cargo test --locked -p moq-sock --all-targets --target x86_64-pc-windows-gnu fails: error[E0463]: can't find crate for &#96;core&#96;; the Windows target is not installed, so the changed Windows code cannot b. Recommendation: Run the moq-sock build and tests on a Windows runner with the required target toolchain before requeueing.

ci: check rs cancelled in 2s
==> check rs
sh/rs/select.sh lint "$LIST"
rs: selecting the workspace
rs: selecting the workspace
python3 sh/rs/stats-docs.py
# The environment is process-wide, so nothing writes it. A text check, because the
# clock lint's test allowances also silence clippy's disallowed_methods.
git grep --untracked -nE '(^|[^[:alnum:]_])(set_var|remove_var)[[:space:]]*\(' -- 'rs/*.rs'; [ $? -eq 1 ] || { echo "error: a write to the process environment: pass the value down, or run a test in a child with test_env::Env" >&2; exit 1; }
cargo fmt --all --check
error: recipe `lint-changed` failed on line 70 with exit code 143
ci: check python failed

(written by codex / Codex)

@kixelated
kixelated added this pull request to the merge queue Nov 2, 2026
Merged via the queue into main with commit a8eb029 Nov 2, 2026
13 checks passed
@kixelated
kixelated deleted the quest/m2/cmsg-windows-truncation branch November 2, 2026 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant