refactor(client): make session opening transactional #43
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Lochnair-Labs/irtt-rs#43
Reference in new issue
Block a user
Summary
Make synchronous session opening transactional for the blocking client, its
threaded managed wrapper, and the legacy managed group.
Opening transaction
SessionMachine now prepares immutable open requests, classifies received
datagrams, prepares complete active/no-test state, and commits opening
infallibly. Socket, scheduling, buffer, DSCP, retry, and cleanup ownership
remain in the adapters.
Trust and retry behavior
Each configured timeout sends exactly one request and establishes one absolute
attempt deadline. Malformed or unauthenticated traffic is ignored within that
attempt. Authenticated semantic incompatibilities remain terminal.
Adapter rollback and cleanup
Blocking and managed adapters complete fallible preparation before machine
commit. Post-token failures send a best-effort preencoded authenticated cleanup
close while preserving the primary error. DSCP and timeout rollback are
best-effort and no partial open state survives failure.
Managed receiver draining
When an opening attempt reaches its deadline, targets wait for a shared
receiver-drain generation before retrying or reporting final timeout. The
receiver completes that generation only after an empty receive observed after
the drain request, ensuring datagrams already queued in the socket are
processed first.
Multiple expired targets share one drain generation. Userspace dequeue
timestamps are no longer treated as kernel arrival times, and completed
attempts cannot be reopened by later packets.
Testing
Adds focused coverage for trust classification, HMAC behavior, absolute
deadlines, request counts, no-test opening, adapter fault injection, cleanup
close behavior, group policy failures, receiver progress, queued in-window
replies, post-deadline replies, and final timeout coordination.
Adds deterministic coverage for queued replies processed after their nominal
deadline, receiver request/receive races, shared drain generations, post-drain
reply rejection, final-timeout coordination, no-test cleanup, and no-test
group-policy separation.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:
29ab07c0f7ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
@@ -9,6 +9,26 @@ fn successful_open_handshake() {On Linux in this repository,
cargo test -p irtt-clientfails this new assertion because the socket reportsSome(76ms)after setting75ms; receive-timeout values may be rounded to the platform timer resolution. The test already verifies the exact value passed to the restoration helper throughlast_restored_read_timeout, so requiring exact kernel readback makes the suite platform-dependent and should be replaced with a tolerance/range check or omitted.Useful? React with 👍 / 👎.
@@ -949,2 +984,4 @@}fn process_group_datagram(socket: &UdpSocket,When the group receiver is delayed or has a socket backlog, an open reply can enter the UDP receive queue before this attempt's deadline but be dequeued afterward;
received_at.monois sampled only afterrecvmsg, so this check rejects that on-time reply as outside the window. The new receiver watermark uses the same userspace timestamp and therefore cannot protect this case, allowing unnecessary retries or a finalOpenTimeoutunder scheduler delays or multi-target load; eligibility needs to use the kernel receive timestamp or another queue-draining scheme that preserves actual arrival ordering.Useful? React with 👍 / 👎.
@@ -951,0 +1077,4 @@datagram.received_at,datagram.meta,);}For a managed group using
RunMode::NoTestwith loose negotiation, a valid close reply that restricts the interval or DSCP now fails here withNegotiationRejected. Before this change,validate_group_negotiationran only through the normal-open callback, and these constraints are irrelevant to no-test sessions because they never schedule probes or apply per-target DSCP; validate onlynormal_negotiated()replies so valid negotiation-only runs still complete.Useful? React with 👍 / 👎.
@@ -121,58 +179,99 @@ impl SessionMachine {self.config.hmac_key.is_some()When a
RunMode::NoTestrequest receives an unexpected non-close reply with a nonzero token, this condition suppresses cleanup solely because the client is not in normal mode;prepare_open_acceptancethen returnsUnexpectedNoTestReply, and both the blocking and group adapters have no close packet to send. A server that allocated the returned token therefore retains an orphaned session, so cleanup should be prepared for any non-close, nonzero-token reply.Useful? React with 👍 / 👎.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:
185e0bf3e4ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
When another active target supplies datagrams more frequently than the group's receive timeout (for example, replies at the supported 10 ms interval),
recv_datagram_fromkeeps succeeding and this is never called. An expired opening target therefore waits forever for its drain generation atdrive_open_attemptsinstead of retransmitting or reachingOpenTimeout, which can prevent the entire mixed group from completing. Drain completion needs an absolute bound or queue-boundary mechanism that does not require a completely idle socket.Useful? React with 👍 / 👎.
When an accepted timeout such as
Duration::MAXcannot be added toInstant::now(), this passesDurationOverflowthroughManagedTargetFailure::opening;classify_target_failurethen falls through toOpeningProtocol, even though no request was sent and no protocol data was processed. This mislabels the target outcome for callers and monitoring, so deadline overflow should be classified asInvalidConfiguration(or otherwise as a local failure) rather than a protocol/authentication/negotiation failure.Useful? React with 👍 / 👎.