Merge pull request 'Fix for destination handler EOF handling' (#22) from fix/duplicate-eof-pdu-not-acknowledged into main

Reviewed-on: #22

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
 

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!";