Skip to content

Commit ad0fdd5

Browse files
committed
fix(git-remote): normalize the shim's accepted socket to blocking (#192)
start_shim sets the listener non-blocking so its accept loop can poll the stop flag. On unix accept() hands back a blocking socket, so that never showed; on Windows the accepted socket inherits the listener's FIONBIO state and std does not reset it, so every connection arrived non-blocking. handle_conn reads with read_line(..).unwrap_or(0), which turns the resulting WouldBlock into "peer closed" and returns without writing a response, dropping the connection and aborting the client mid-request. The shim accepts as soon as the handshake completes, strictly before the request bytes arrive, so the window was open on every connection and closed only because the 5ms accept poll usually lagged the client. That is what made real_git_withheld_shaped_first_post fail on windows-latest with os error 10053 and zero POSTs recorded. Normalizing at handle_conn entry also restores the 30s read timeout on the next line, which a non-blocking socket silently made meaningless. The regression test forces the Windows socket shape explicitly, so it is load-bearing on every platform rather than only the one that exhibits the bug, and it establishes the accept-before-request interleaving with a handshake rather than a sleep: a timing-based version cannot flake red but can flake green under load, which would make the guard vacuous.
1 parent a53c151 commit ad0fdd5

1 file changed

Lines changed: 88 additions & 0 deletions

File tree

‎crates/git-remote-gitlawb/tests/real_git_fetch.rs‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,14 @@ fn start_shim(repo: PathBuf, mode: ShimMode) -> Shim {
163163
}
164164

165165
fn handle_conn(stream: TcpStream, repo: &Path, mode: ShimMode, posts: &AtomicUsize) {
166+
// `start_shim` makes the LISTENER non-blocking to poll its stop flag. On
167+
// Windows the accepted socket inherits that (Winsock FIONBIO inheritance;
168+
// std's `accept` does not reset it), on unix it does not. Every read below
169+
// is a blocking read whose errors collapse to "peer closed", so an inherited
170+
// WouldBlock would drop the connection with no response and abort the client
171+
// mid-request. Normalize before reading; this also restores the read timeout
172+
// on the next line, which a non-blocking socket silently makes meaningless.
173+
stream.set_nonblocking(false).ok();
166174
stream.set_read_timeout(Some(Duration::from_secs(30))).ok();
167175
let mut reader = BufReader::new(stream);
168176

@@ -749,6 +757,86 @@ fn build_divergent_repos(shared_commits: usize) -> Repos {
749757
}
750758
}
751759

760+
/// The shim must answer a connection that arrives in non-blocking mode.
761+
///
762+
/// `start_shim` sets the LISTENER non-blocking so the accept loop can poll the
763+
/// stop flag. On unix the socket handed back by `accept()` is blocking, so that
764+
/// is invisible; on Windows, Winsock has the accepted socket inherit the
765+
/// listener's FIONBIO state and std does not reset it (`accept` is a bare
766+
/// `c::accept`), so every accepted connection is non-blocking there. The shim
767+
/// reads with `read_line(..).unwrap_or(0)`, which collapses `WouldBlock` into
768+
/// "peer closed" and returns WITHOUT writing a response, aborting the client
769+
/// mid-request. Because the shim accepts as soon as the handshake completes,
770+
/// strictly before the request bytes arrive, that window is open on every
771+
/// connection and closes only because the accept poll usually lags the client.
772+
///
773+
/// Force the Windows shape explicitly so this is checked on every platform
774+
/// rather than only the one that exhibits it. The accept-before-request
775+
/// interleaving is the losing one, so it is established by a handshake rather
776+
/// than a sleep: a timing-based version cannot flake RED, but it CAN flake
777+
/// GREEN on a loaded runner (bytes already buffered, so the read succeeds even
778+
/// while non-blocking), which would make this guard vacuous exactly where it
779+
/// is supposed to bite.
780+
#[test]
781+
fn shim_answers_a_connection_that_arrives_non_blocking() {
782+
let repos = build_divergent_repos(1);
783+
let repo = repos.server.clone();
784+
785+
let listener = TcpListener::bind("127.0.0.1:0").unwrap();
786+
listener.set_nonblocking(true).unwrap();
787+
let addr = listener.local_addr().unwrap();
788+
let (accepted_tx, accepted_rx) = std::sync::mpsc::channel();
789+
790+
let server = std::thread::spawn(move || {
791+
let posts = AtomicUsize::new(0);
792+
let stream = loop {
793+
match listener.accept() {
794+
Ok((s, _)) => break s,
795+
Err(ref e) if e.kind() == std::io::ErrorKind::WouldBlock => {
796+
std::thread::sleep(Duration::from_millis(5))
797+
}
798+
Err(e) => panic!("accept failed: {e}"),
799+
}
800+
};
801+
// What Windows hands the shim. On unix this is the only way to reach
802+
// that state, so the assertion below is load-bearing here too.
803+
stream.set_nonblocking(true).unwrap();
804+
accepted_tx.send(()).unwrap();
805+
handle_conn(stream, &repo, ShimMode::Normal, &posts);
806+
});
807+
808+
let mut client = TcpStream::connect(addr).unwrap();
809+
// Send only once the shim has the connection in hand, so its first read is
810+
// guaranteed to run against an empty receive buffer.
811+
accepted_rx.recv().unwrap();
812+
client
813+
.write_all(b"GET /x/info/refs?service=git-upload-pack HTTP/1.1\r\nHost: x\r\n\r\n")
814+
.unwrap();
815+
816+
let mut got = Vec::new();
817+
let read = client.read_to_end(&mut got);
818+
server.join().unwrap();
819+
820+
assert!(
821+
read.is_ok(),
822+
"client read failed instead of receiving a response: {:?} \
823+
(a non-blocking accepted socket makes the shim drop the connection \
824+
with no reply, which surfaces as a connection abort on the client)",
825+
read.unwrap_err().kind()
826+
);
827+
assert!(
828+
got.starts_with(b"HTTP/1.1 200 OK"),
829+
"shim closed the connection without a response ({} bytes read); \
830+
a WouldBlock on the request-line read was swallowed as EOF",
831+
got.len()
832+
);
833+
assert!(
834+
got.windows(b"# service=git-upload-pack".len())
835+
.any(|w| w == b"# service=git-upload-pack"),
836+
"response reached the client but is not the upload-pack advertisement"
837+
);
838+
}
839+
752840
/// Matrix item 7, bridging half: a real multi-round `git fetch` through the
753841
/// helper against a real `git upload-pack --stateless-rpc` completes, is observed
754842
/// at >=2 POSTs (the executable form of the trigger; a fixture that resolved in

0 commit comments

Comments
 (0)