fix(s2n-quic-transport): treat clean connection close as dc complete

[boquan-fang]

Release Summary:

Fix a bug that server never receives its ACK and therefore not moving to the dc complete state.

Resolved issues:

resolves #3055.

Description of changes:

Currently, the server must receive the ACK of the DC_STATELESS_RESET_TOKEN in order for it to move on to dc complete:

s2n-quic/quic/s2n-quic-transport/src/dc/manager.rs

Lines 206 to 220 in 1c11f00

	/// Called when a range of packets have been acknowledged
	pub fn on_packet_ack<A: ack::Set, Pub: event::ConnectionPublisher>(
	&mut self,
	ack_set: &A,
	publisher: &mut Pub,
	) {
	ensure!(self.stateless_reset_token_sync.on_packet_ack(ack_set));
	ensure!(self.state.on_stateless_reset_tokens_acked().is_ok());
	debug_assert!(Config::ENDPOINT_TYPE.is_server());
	self.path.on_dc_handshake_complete();
	publisher.on_dc_state_changed(DcStateChanged {
	state: DcState::Complete,
	});
	}

However, as specified by #3055, under heavy packet losses or packet reordering, the connection might already be closed before the ACK is received by the server. Hence, the server remains at ServerTokensSent state instead of Complete state.

The fix that this PR proposed is to treat a clean CONNECTION_CLOSE as the proof that server can move on to dc complete state.

Analysis of such fix:

The assumption of receiving a clean CONNECTION_CLOSE is based on how the client finishes the handshake and ConfirmComplete. For the client, it will only reaches dc state complete once the DC_STATELESS_RESET_TOKEN is received:

s2n-quic/quic/s2n-quic-transport/src/dc/manager.rs

Lines 196 to 203 in 1c11f00

	if Config::ENDPOINT_TYPE.is_server() {
	self.stateless_reset_token_sync.send();
	} else {
	self.path.on_dc_handshake_complete();
	publisher.on_dc_state_changed(DcStateChanged {
	state: DcState::Complete,
	});
	}

The client would close the connection only once ConfirmComplete::wait_ready finished running.

Therefore, on the happy path, the server can conclude that receiving a clean CONNECTION_CLOSE frame from the client means the server's token was received by the client.

Changes we have to make for the soundness of the assumption

There is a situation where the client sends a clean CONNECTION_CLOSE without actually receiving the token. According to the io's handshake function

s2n-quic/dc/s2n-quic-dc/src/psk/io.rs

Lines 529 to 546 in 1c11f00

	match tokio::time::timeout_at(deadline, ConfirmComplete::wait_ready(&mut connection))
	.await
	{
	Ok(Ok(())) => {
	// ConfirmComplete succeeded within the deadline - continue
	}
	Ok(Err(e)) => {
	// ConfirmComplete::wait_ready failed. We should treat the handshake as failed.
	return Err(e);
	}
	Err(_elapsed) => {
	// Handshake timeout occurred. We should treat the handshake as failed.
	return Err(io::Error::new(
	io::ErrorKind::TimedOut,
	"ConfirmComplete handshake timeout",
	));
	}
	}

The client's ConfirmComplete::wait_ready might timeout or errors, but the handshake function would still cleanly close the connection, because the Drop impl for Connection is set to always close the connection with no errors:

s2n-quic/quic/s2n-quic-transport/src/connection/api.rs

Lines 86 to 103 in 1c11f00

	impl Clone for Connection {
	fn clone(&self) -> Self {
	// Safety
	//
	// Using a relaxed ordering is alright here, as knowledge of the
	// original reference prevents other threads from erroneously deleting
	// the object.
	// https://github.com/rust-lang/rust/blob/e012a191d768adeda1ee36a99ef8b92d51920154/library/alloc/src/sync.rs#L1329
	self.api
	.application_handle_count()
	.fetch_add(1, Ordering::Relaxed);
	Self {
	api: self.api.clone(),
	// don't clone the open token - each instance should have its own token
	open_token: OpenToken::new(),
	}
	}
	}

Hence, on this error path, our assumption that receiving a clean CONNECTION_CLOSE means the client has received the token is wrong.

However, that behavior sounds wrong: if the client determines the handshake failed as specified by the comments, it should explicitly close the connection with an error code instead of closing the connection with no error code. This PR changes that behavior: the client should explicitly call .close(error_code) if it determines the handshake wasn't successful.

Call-outs:

DC_HANDSHAKE_INCOMPLETE_ERROR

I set up a DC_HANDSHAKE_INCOMPLETE_ERROR code for the connection closure while handshake failed. I don't think there is a proper error code in s2n-quic can be reused for our usecase.

Test structure

Since I need to configure the server and client in a certain way: specifically need to add a packet interceptor for test purposes, I can not reuse the test helper functions like self_test_inner. I add another test helper function dc_completes_through_close to perform the test for this PR.

Also, I basically drop all standalone ACKs for the server, so MTU probing can't be performed. I pinned all MTU settings to be the same value to avoid probing.

Testing:

I add two integration tests:

1. Remove all standalone ACKs to see if both endpoint can reach dc complete state.
2. Test 1 + remove the first connection close packet that the server received and see if the server can still reach dc complete.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.