Skip to content

refactor(proto): do not set timers in poll_transmit #721

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flub wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

refactor(proto): do not set timers in poll_transmit #721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f9931ee
refactor(proto): do not set timers in poll_transmit
flub Jun 24, 2026
c4f853b
remove unused state
flub Jun 25, 2026
4e5eb9b
move on-path challenge pto timer back to send time
flub Jun 25, 2026
b3f2fda
Remove PathValidationFailed time, use path timeout instead
flub Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 23 additions & 39 deletions noq-proto/src/connection/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,7 +590,7 @@ impl Connection {
return Err(PathError::RemoteCidsExhausted);
}

let path = self.ensure_path(path_id, network_path, now, None);
let path = self.create_path(path_id, network_path, now, None);
path.status.local_update(initial_status);

Ok(path_id)
Expand Down Expand Up @@ -717,9 +717,7 @@ impl Connection {
// These timers deal with sending and receiving PATH_CHALLENGE and
// PATH_RESPONSE, but now that the path is abandoned, we no longer care about
// these frames or their timing
PathTimer::PathValidationFailed
| PathTimer::PathChallengeLost
| PathTimer::AbandonFromValidation => false,
PathTimer::PathValidationFailed | PathTimer::PathChallengeLost => false,
// These timers deal with the lifetime of the path. Now that the path is abandoned,
// these are not relevant.
PathTimer::PathKeepAlive | PathTimer::PathIdle => false,
Expand Down Expand Up @@ -929,7 +927,7 @@ impl Connection {
/// Creates the [`PathData`] for a new [`PathId`].
///
/// Called for incoming packets as well as when opening a new path locally.
fn ensure_path(
fn create_path(
&mut self,
path_id: PathId,
network_path: FourTuple,
Expand Down Expand Up @@ -978,6 +976,18 @@ impl Connection {
.address_discovery_role
.should_report(&self.peer_params.address_discovery_role);

// if !data.validated {
// // Hard cap on how long to try validation for.
// // TODO(flub): 3 * PTO is way too short, it makes opening paths very susceptible
// // to packet loss. I prefer using the path idle timeout probably.
// let pto = self.ack_frequency.max_ack_delay_for_pto() + data.rtt.pto_base();
// self.timers.set(
// Timer::PerPath(path_id, PathTimer::AbandonFromValidation),
// now + 3 * pto,
// self.qlog.with_time(now),
// );
// }

let path = vacant_entry.insert(PathState { data, prev: None });

let mut pn_space = spaces::PacketNumberSpace::new(now, SpaceId::Data, &mut self.rng);
Expand Down Expand Up @@ -2527,24 +2537,11 @@ impl Connection {
trace!(?path.data.lost_challenge_count, "path challenge deemed lost");
path.data.pending_challenge = true;
path.data.lost_challenge_count += 1;
}
PathTimer::AbandonFromValidation => {
let Some(path) = self.paths.get_mut(&path_id) else {
continue;
};
path.data.reset_on_path_challenges();
self.timers.stop(
self.timers.set(
Timer::PerPath(path_id, PathTimer::PathChallengeLost),
now + path.data.on_path_challenge_pto(),
self.qlog.with_time(now),
);
debug!("new path validation failed");
if let Err(err) = self.close_path_inner(
now,
path_id,
PathAbandonReason::ValidationFailed,
) {
warn!(?err, "failed closing path");
}
}
PathTimer::Pacing => {}
PathTimer::MaxAckDelay => {
Expand Down Expand Up @@ -4349,7 +4346,7 @@ impl Connection {

if self.side().is_server() && !self.abandoned_paths.contains(&path_id) {
// Only the client is allowed to open paths
self.ensure_path(path_id, network_path, now, pn);
self.create_path(path_id, network_path, now, pn);
}
if self.paths.contains_key(&path_id) {
self.on_packet_authenticated(
Expand Down Expand Up @@ -5622,10 +5619,6 @@ impl Connection {
Timer::PerPath(path_id, PathTimer::PathValidationFailed),
qlog.clone(),
);
self.timers.stop(
Timer::PerPath(path_id, PathTimer::AbandonFromValidation),
qlog.clone(),
);
let next_challenge = path
.data
.earliest_on_path_expiring_challenge()
Expand Down Expand Up @@ -6179,23 +6172,14 @@ impl Connection {
let challenge = frame::PathChallenge(token);
builder.write_frame(challenge, stats);
builder.require_padding();
let pto = self.ack_frequency.max_ack_delay_for_pto() + path.rtt.pto_base();
let pns = space.for_path(path_id);
match pns.open_status {
OpenStatus::Sent | OpenStatus::Informed => {}
OpenStatus::Pending => {
pns.open_status = OpenStatus::Sent;
self.timers.set(
Timer::PerPath(path_id, PathTimer::AbandonFromValidation),
now + 3 * pto,
self.qlog.with_time(now),
);
}
}

// On-path challenges need a PATH_RESPONSE and not only an ACK which can be
// received on any path. So set a timer manually instead of relying on the usual
// LossDetection/PTO timer. This timer interval keeps exponentially increasing
// with missing responses like the normal PTO interval.
self.timers.set(
Timer::PerPath(path_id, PathTimer::PathChallengeLost),
now + path.on_path_challenge_expiry(),
now + path.on_path_challenge_pto(),
self.qlog.with_time(now),
);

Expand Down
16 changes: 14 additions & 2 deletions noq-proto/src/connection/paths.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,14 +477,22 @@ impl PathData {
if self.unconfirmed_challenges.is_empty() {
return None;
}
let duration = self.on_path_challenge_expiry();
let duration = self.on_path_challenge_pto();
self.unconfirmed_challenges
.values()
.map(|info| info.sent_instant + duration)
.min()
}

pub(super) fn on_path_challenge_expiry(&self) -> Duration {
/// The duration after which a PTO expires for an on-path challenge, if sent now.
///
/// Since challenges need an on-path response rather than just an ACK that can be sent
/// on any path they need a different timer from the
/// [`PathTimer::LossDetection`]. Functionally this behaves as the probe timeout
/// however.
///
/// [`PathTimer::LossDetection`]: super::timer::PathTimer::LossDetection
pub(super) fn on_path_challenge_pto(&self) -> Duration {
let backoff = 2u32.pow(self.lost_challenge_count.min(MAX_BACKOFF_EXPONENT));
let duration = self.rtt.pto_base() * backoff;
duration.min(MAX_PTO_INTERVAL)
Expand Down Expand Up @@ -1095,6 +1103,10 @@ pub enum PathAbandonReason {
error_code: VarInt,
},
/// We didn't receive a path response in time after opening this path.
///
/// This event is no longer emitted, when validation fails a path is only abandoned once
/// there's a path timeout and the [`Self::TimedOut`] event would be emitted instead.
// TODO(flub): should this be deprecated?
ValidationFailed,
/// We didn't receive any data from the remote within the path's idle timeout.
TimedOut,
Expand Down
3 changes: 0 additions & 3 deletions noq-proto/src/connection/qlog.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,9 +323,6 @@ impl QlogSink {
PathTimer::PathIdle => Some(TimerType::custom("path_idle")),
PathTimer::PathValidationFailed => Some(QlogTimerType::PathValidation.into()),
PathTimer::PathChallengeLost => Some(TimerType::custom("path_challenge_lost")),
PathTimer::AbandonFromValidation => {
Some(TimerType::custom("abandon_from_validation"))
}
PathTimer::PathKeepAlive => Some(TimerType::custom("path_keep_alive")),
PathTimer::Pacing => Some(TimerType::custom("pacing")),
PathTimer::MaxAckDelay => Some(QlogTimerType::Ack.into()),
Expand Down
5 changes: 1 addition & 4 deletions noq-proto/src/connection/spaces.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,12 +496,9 @@ impl PacketNumberSpace {

#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
pub(super) enum OpenStatus {
/// A first packet has not been sent using this [`PathId`].
/// The application has not yet been informed of this path.
#[default]
Pending,
/// The first packet has been sent using this [`PathId`]. However, it is not yet deemed good
/// enough to be reported to the application.
Sent,
/// The application has been informed of this path.
Informed,
}
Expand Down
20 changes: 5 additions & 15 deletions noq-proto/src/connection/timer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,32 +60,22 @@ pub(crate) enum PathTimer {
PathValidationFailed = 2,
/// When to resend an on-path path challenge deemed lost
PathChallengeLost = 3,
/// When to abandon a path due to failed validation.
///
/// There are two situations in which we give up a path from validation.
/// 1. When opening a path we validate it according to RFC9000 §8.2 as required by the
/// multipath spec. This timer is armed to time-bound that validation.
/// 2. When validating an already opened multipath path for various reasons in which we
/// expect the path to either work or not work and want to respond correspondingly in
/// a timely manner.
AbandonFromValidation = 4,
/// When to send a `PING` frame to keep the path alive
PathKeepAlive = 5,
PathKeepAlive = 4,
/// When pacing will allow us to send a packet
Pacing = 6,
Pacing = 5,
/// When to send an immediate ACK if there are unacked ack-eliciting packets of the peer
MaxAckDelay = 7,
MaxAckDelay = 6,
/// When to clean up state for an abandoned path
PathDrained = 8,
PathDrained = 7,
}

impl PathTimer {
pub(super) const VALUES: [Self; 9] = [
pub(super) const VALUES: [Self; 8] = [
Self::LossDetection,
Self::PathIdle,
Self::PathValidationFailed,
Self::PathChallengeLost,
Self::AbandonFromValidation,
Self::PathKeepAlive,
Self::Pacing,
Self::MaxAckDelay,
Expand Down
66 changes: 0 additions & 66 deletions noq-proto/src/tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1506,72 +1506,6 @@ fn migration() {
);
}

#[test]
fn path_challenge_retransmit() {
let _guard = subscribe();
let mut pair = Pair::default();
let (client_ch, server_ch) = pair.connect();
pair.drive();

pair.client_conn_mut(client_ch).ping();
pair.drive();

println!("-------- server wants path validation --------");
pair.server_conn_mut(server_ch).trigger_path_validation();
pair.drive_server(); // Send the path challenge
println!("-------- client loses messages --------");
// Have the client lose the challenge
pair.client.inbound.clear();

pair.drive();

let client_tx = pair.client_conn_mut(client_ch).stats().frame_tx;
let server_tx = pair.server_conn_mut(server_ch).stats().frame_tx;

assert_eq!(
server_tx.path_challenge, 2,
"expected server to send two path challenges"
);
assert_eq!(
client_tx.path_response, 1,
"expected client to send one path response"
);
}

#[test]
fn path_response_retransmit() {
let _guard = subscribe();
let mut pair = Pair::default();
let (client_ch, server_ch) = pair.connect();
pair.drive();

pair.client_conn_mut(client_ch).ping();
pair.drive();

println!("-------- server wants path validation --------");
pair.server_conn_mut(server_ch).trigger_path_validation();
pair.drive_server(); // Send the path challenge
pair.drive_client(); // Send the path response
println!("-------- server loses messages --------");
// Have the server lose the path response
pair.server.inbound.clear();

// The server should decide to re-send the path challenge
pair.drive();

let client_tx = pair.client_conn_mut(client_ch).stats().frame_tx;
let server_tx = pair.server_conn_mut(server_ch).stats().frame_tx;

assert_eq!(
server_tx.path_challenge, 2,
"expected server to send two path challenges"
);
assert_eq!(
client_tx.path_response, 2,
"expected client to send two path responses"
);
}

/// Regression test: handle sent challenges that are waiting for a response while a passive
/// migration occurs.
///
Expand Down
Loading
Loading