try_recv_timeout loses messages on Windows #450

AI Caveat

This bug was diagnosed by Claude Code. A test to reproduce the behaviour was added to draft PR #449. I reviewed the test and it looks legitimate. The test failed in this run:

thread 'test::try_recv_timeout_message_not_lost_at_boundary' (1180) panicked at src\test.rs:808:14:
message lost: first try_recv_timeout returned Empty but message is no longer in the pipe: IpcError(Disconnected)

A putative fix produced a clean run. I cannot vouch for the fix because I lack the necessary Windows expertise (and any desire to obtain it).

If Servo's AI policy applies to this repository, the test and fix should be handled with caution. However, I believe having this bug on the books is beneficial in case someone else encounters it.

Summary

On Windows, OsIpcReceiver::try_recv_timeout can silently discard data that the kernel has already read from the pipe. This happens when a timeout and a completed read race in MessageReader::issue_async_cancel.

Environment

• ipc-channel 0.21.0
• Windows (x86_64 and i686)
• Named pipe in byte mode (PIPE_TYPE_BYTE | PIPE_READMODE_BYTE)

Root cause

MessageReader::issue_async_cancel (platform/windows/mod.rs, line 657) calls CancelIoEx to cancel a pending overlapped ReadFile. When the read has already completed by the time CancelIoEx runs, Windows returns ERROR_NOT_FOUND (there is no outstanding operation to cancel). The handler for this case (line 683) restores the buffer from the async data:

let async_data = self.r#async.take().unwrap().into_inner();
self.handle = async_data.handle;
self.read_buf = async_data.buf;

However, async_data.buf.len() is still the pre-read length that was set in start_read (line 781). The bytes the kernel wrote into the buffer are present in memory beyond the visible length, but the length is never updated to expose them. The normal completion path through notify_completion does update the length (set_len(buf_len + nbytes)), but the ERROR_NOT_FOUND path bypasses notify_completion entirely.

Because the pipe is in byte mode, the bytes consumed by the kernel's ReadFile are removed from the pipe. Since the buffer length is not updated, those bytes become permanently inaccessible. The next call to start_read overwrites them in the buffer.

Race sequence

1. try_recv_timeout(d) calls receive_message(Timeout(d))
2. start_read begins an overlapped ReadFile on the pipe
3. fetch_async_result calls GetOverlappedResultEx with timeout d
4. The timeout expires — WAIT_TIMEOUT — before data arrives
5. cancel_io → issue_async_cancel → CancelIoEx
6. Between step 4 and step 5, the sender writes a message and the kernel
   completes the read
7. CancelIoEx returns ERROR_NOT_FOUND
8. issue_async_cancel restores the buffer without updating its length —
   the read data is silently lost
9. cancel_io sees self.async is None, skips fetch_async_result
10. fetch_async_result returns Err(NoData) → caller sees TryRecvError::Empty

On the next receive attempt, the lost message's bytes are gone from the pipe and invisible in the buffer. If a second message was written after the first, the receiver sees the second message first, breaking FIFO ordering.

Reproducer

Any use of try_recv_timeout where a message arrives near the timeout boundary can trigger this. A minimal sketch:

use ipc_channel::ipc;
use std::thread;
use std::time::Duration;

let (tx, rx) = ipc::channel::<i32>().unwrap();

thread::spawn(move || {
    // Sleep for approximately the same duration as the timeout
    thread::sleep(Duration::from_millis(100));
    tx.send(1).unwrap();
    tx.send(2).unwrap();
});

// First message may be silently lost if it arrives during the cancel window
let val = rx.try_recv_timeout(Duration::from_millis(100));
// val may be Err(Empty) even though message 1 was written to the pipe

let val = rx.try_recv_timeout(Duration::from_millis(5000));
// val may be Ok(2), with message 1 permanently lost

The race window is small, so the bug is intermittent. It is more likely when the timeout duration is close to the sender's latency.

Suggested fix

In issue_async_cancel, when CancelIoEx returns ERROR_NOT_FOUND, the overlapped operation has already completed successfully. The code should retrieve the number of bytes transferred (from ov.InternalHigh or via GetOverlappedResult) and update read_buf's length accordingly, just as notify_completion does for the normal completion path.

One approach: call notify_completion(Ok(())) instead of manually extracting the async data:

assert!(error.code() == ERROR_NOT_FOUND.to_hresult());
// The operation completed before we could cancel it.
// Process it as a normal completion.
self.notify_completion(Ok(()))
    .expect("completed read should not fail");

When the bug was introduced

The bug was introduced in commit eb08381a, dated 2021-11-17, with the message: "add try_recv_timeout method, implement for Windows".

The issue_async_cancel method's ERROR_NOT_FOUND handler has always restored the buffer without updating its length to account for bytes the kernel wrote. However, prior to this commit, that was harmless — cancel_io() was only called during Drop, where the reader would never be used again, so losing buffered data didn't matter.

Commit eb08381a added a new code path in fetch_async_result for the WAIT_TIMEOUT case:

if err == winapi::shared::winerror::WAIT_TIMEOUT {
    self.cancel_io();
    return Err(WinError::NoData);
}

This calls cancel_io() and then continues using the reader for subsequent receives. When the async read completes between the timeout expiring and CancelIoEx running, CancelIoEx returns ERROR_NOT_FOUND, and issue_async_cancel restores the buffer without updating its length — silently discarding the data the kernel already read. The normal completion path through notify_completion calls set_len(buf_len + nbytes), but the ERROR_NOT_FOUND path bypasses that entirely.

In short: the latent flaw in issue_async_cancel became a real bug the moment cancel_io() started being called in a context where the reader lives on.