From b74893f6c7ad37c67029540c59d09e1edceb2732 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Mon, 14 Sep 2026 11:28:49 +0200 Subject: [PATCH] Fix for destination handler EOF handling Destination handler now answers a retransmitted EOF PDU instead of silently reprocessing it. Per CFDP 4.7.2 every EOF PDU must be acknowledged, and a retransmission means our previous ACK was lost. It used to be handled like a brand new EOF regardless of transaction state, which re-ran completion, re-sent a fresh Finished PDU, and restarted its positive ACK procedure on every retransmission the sender made. --- CHANGELOG.md | 5 ++ src/dest.rs | 127 ++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 126 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55c7d42..6f046cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/). recently received window instead of its end, so such a retransmission matched neither branch and the gap stayed in the tracker, causing the destination to re-request data it had already written on every NAK round until it hit its NAK limit. +- Destination handler now answers a retransmitted EOF PDU instead of silently reprocessing it. + Per CFDP 4.7.2 every EOF PDU must be acknowledged, and a retransmission means our previous ACK + was lost. It used to be handled like a brand new EOF regardless of transaction state, which + re-ran completion, re-sent a fresh Finished PDU, and restarted its positive ACK procedure on + every retransmission the sender made. # [v0.3.0] 2025-09-25 diff --git a/src/dest.rs b/src/dest.rs index 511ff8c..dba1e01 100644 --- a/src/dest.rs +++ b/src/dest.rs @@ -994,6 +994,15 @@ impl< let first_packet = self.step() == TransactionStep::Idle; if first_packet { self.first_packet_handling(eof_pdu.pdu_header().common_pdu_conf())?; + } else if self.is_duplicate_eof_pdu() { + // CFDP 4.7.2: every EOF PDU must be acknowledged. Reaching this point means our + // previous ACK did not arrive, because the sender only retransmits the EOF on its + // own positive ACK timer. Answering it again is the only thing that breaks the + // deadlock: otherwise the sender retransmits to its limit and declares a fault at + // the end of an otherwise successful transfer. Nothing else about the transaction + // is touched, the EOF was already fully processed when the first copy arrived. + self.acknowledge_eof_pdu(&eof_pdu)?; + return Ok(1); } if self.local_cfg.indication_cfg.eof_recv { // Unwrap is okay here, application logic ensures that transaction ID is valid here. @@ -1043,6 +1052,25 @@ impl< Ok(sent_packets) } + /// Whether this is an EOF PDU for a step which already consumed one. + /// + /// `deferred_procedure_active` is only ever set by an EOF handler, so it being active + /// already means an EOF was processed before. The other reachable case is waiting for the + /// ACK of our own Finished PDU, sent once the transfer (and the deferred procedure, if any) + /// completed. + fn is_duplicate_eof_pdu(&self) -> bool { + if self.transaction_params.transmission_mode() != TransmissionMode::Acknowledged { + return false; + } + if self.step() == TransactionStep::WaitingForFinishedAck { + return true; + } + self.transaction_params + .acked_params + .as_ref() + .is_some_and(|params| params.deferred_procedure_active) + } + fn handle_eof_without_previous_metadata_in_acked_mode( &mut self, eof_pdu: &EofPdu, @@ -3061,12 +3089,7 @@ mod tests { .lost_segment_tracker .add_lost_segment((8, 12)) .expect("adding lost segment failed"); - let acked_params = tb - .handler - .transaction_params - .acked_params - .as_mut() - .unwrap(); + let acked_params = tb.handler.transaction_params.acked_params.as_mut().unwrap(); acked_params.last_start_offset = 8; acked_params.last_end_offset = 12; @@ -3079,6 +3102,98 @@ mod tests { assert!(tb.handler.lost_segment_tracker.is_empty()); } + /// CFDP 4.7.2: every EOF PDU must be acknowledged. If our ACK is lost, the sender + /// retransmits the EOF until its positive ACK limit, so a destination which has already + /// moved on to waiting for the Finished ACK still has to answer it. + #[test] + fn test_duplicate_eof_is_acknowledged_while_waiting_for_finished_ack() { + let fault_handler = TestFaultHandler::default(); + let mut tb = DestHandlerTestbench::new_with_fixed_paths( + fault_handler, + TransmissionMode::Acknowledged, + false, + ); + let mut user = tb.test_user_from_cached_paths(0); + let transfer_info = tb + .generic_transfer_init(&mut user, 0) + .expect("transfer init failed"); + tb.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus); + tb.generic_eof_no_error(&mut user, Vec::new()) + .expect("EOF no error insertion failed"); + tb.check_completion_indication_success(&mut user); + assert_eq!(tb.pdu_queue_len(), 2); + tb.check_eof_ack_pdu(ConditionCode::NoError); + tb.check_finished_pdu_success(); + tb.state_check(State::Busy, TransactionStep::WaitingForFinishedAck); + + // The sender never saw our ACK and retransmits the EOF PDU. + let pdu_header = PduHeader::new_for_file_directive(tb.pdu_conf, 0); + let eof_pdu = create_no_error_eof(&[], &pdu_header); + let packet_info = create_packet_info(&eof_pdu, &mut tb.buf); + let sent_packets = tb + .handler + .state_machine(&mut user, Some(&packet_info)) + .expect("duplicate EOF insertion failed"); + assert_eq!(sent_packets, 1); + tb.state_check(State::Busy, TransactionStep::WaitingForFinishedAck); + tb.check_eof_ack_pdu(ConditionCode::NoError); + // The duplicate must not re-run the completion procedures. + assert_eq!(user.finished_indic_queue.len(), 0); + assert_eq!(user.eof_recvd_call_count, 1); + + tb.acknowledge_finished_pdu(&mut user, &transfer_info); + } + + /// Same scenario as above, but the duplicate arrives while the deferred lost segment + /// procedure for a still-missing file segment is running. + #[test] + fn test_duplicate_eof_is_acknowledged_while_deferred_procedure_active() { + let file_data_str = "Hello World!"; + let file_data = file_data_str.as_bytes(); + let file_size = file_data.len() as u64; + let fault_handler = TestFaultHandler::default(); + let mut tb = DestHandlerTestbench::new_with_fixed_paths( + fault_handler, + TransmissionMode::Acknowledged, + false, + ); + tb.remote_cfg_mut().immediate_nak_mode = false; + let mut user = tb.test_user_from_cached_paths(file_size); + let transfer_info = tb + .generic_transfer_init(&mut user, file_size) + .expect("transfer init failed"); + tb.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus); + tb.generic_file_data_insert(&mut user, 0, &file_data[0..5]) + .expect("file data insertion failed"); + tb.generic_eof_no_error(&mut user, file_data.to_vec()) + .expect("EOF no error insertion failed"); + tb.check_eof_ack_pdu(ConditionCode::NoError); + // The NAK for the still-missing segment (5..12). + tb.get_next_pdu(); + assert!(tb.pdu_queue_empty()); + tb.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus); + + // The sender never saw our ACK and retransmits the EOF PDU while the deferred + // procedure for the remaining gap is still running. + let pdu_header = PduHeader::new_for_file_directive(tb.pdu_conf, 0); + let eof_pdu = create_no_error_eof(file_data, &pdu_header); + let packet_info = create_packet_info(&eof_pdu, &mut tb.buf); + let sent_packets = tb + .handler + .state_machine(&mut user, Some(&packet_info)) + .expect("duplicate EOF insertion failed"); + assert_eq!(sent_packets, 1); + tb.check_eof_ack_pdu(ConditionCode::NoError); + tb.state_check(State::Busy, TransactionStep::ReceivingFileDataPdus); + + // The remaining segment still completes the transfer normally. + tb.generic_file_data_insert(&mut user, 5, &file_data[5..]) + .expect("file data insertion failed"); + tb.check_completion_indication_success(&mut user); + tb.check_finished_pdu_success(); + tb.acknowledge_finished_pdu(&mut user, &transfer_info); + } + #[test] fn test_checksum_failure_not_acked() { let file_data_str = "Hello World!";