refactor(stow): handle storage outcome correctly

deny.toml

@@ -7,6 +7,7 @@ unsound = "all"
 ignore = [
     { id = "RUSTSEC-2021-0153", reason = "encoding is used by dicom-encoding (dicom-rs); no maintained replacement upstream yet" },
     { id = "RUSTSEC-2024-0436", reason = "paste is used by rav1e (image, via dicom-pixeldata); proc-macro only" },
+    { id = "RUSTSEC-2026-0440", reason = "astral-tokio-tar is used by testcontainers (dev-dependency only); no testcontainers release without it yet" },
 ]
 
 [licenses]

src/api/stgcmt/service.rs

@@ -1,5 +1,6 @@
+pub use crate::api::stow::FailedInstance;
 use crate::api::stow::InstanceReference;
-use crate::types::{UI, US};
+use crate::types::UI;
 use async_trait::async_trait;
 use axum::body::Body;
 use axum::http::{Response, StatusCode};
@@ -106,13 +107,6 @@ pub struct InstanceLocation {
 	pub series_instance_uid: UI,
 }
 
-/// An instance for which storage has not been committed.
-#[derive(Debug, Clone)]
-pub struct FailedInstance {
-	pub reference: InstanceReference,
-	pub failure_reason: US,
-}
-
 /// The result of a storage commitment request, once known.
 /// Storage Commitment Response Module.
 /// <https://dicom.nema.org/medical/dicom/current/output/chtml/part18/chapter_J.html#sect_J.2>

src/api/stow/routes.rs

@@ -63,10 +63,11 @@ async fn studies(
 		instances: receiver.boxed(),
 	};
 	let response = stow.store(request).await?;
+	let status = response.status_code();
 	let json = DicomJson::from(InMemDicomObject::from(response));
 
 	Ok(Response::builder()
-		.status(StatusCode::OK)
+		.status(status)
 		.header(CONTENT_TYPE, mime::APPLICATION_JSON.as_ref())
 		.body(Body::from(serde_json::to_string(&json).unwrap()))
 		.unwrap())

src/api/stow/service.rs

@@ -1,4 +1,4 @@
-use crate::types::UI;
+use crate::types::{UI, US};
 use async_trait::async_trait;
 use axum::body::Body;
 use axum::http::{Response, StatusCode};
@@ -25,12 +25,47 @@ pub struct InstanceReference {
 	pub sop_instance_uid: UI,
 }
 
+/// An instance that could not be stored, along with the reason.
+#[derive(Debug, Clone)]
+pub struct FailedInstance {
+	pub reference: InstanceReference,
+	/// Failure Reason (0008,1197)
+	pub failure_reason: US,
+}
+
+/// Failure Reason values of the Store Instances Response Module.
+/// <https://dicom.nema.org/medical/dicom/current/output/chtml/part18/chapter_I.html>
+pub mod failure_reason {
+	use crate::types::US;
+
+	/// The origin server failed to store the instance for an unspecified reason.
+	pub const PROCESSING_FAILURE: US = 0x0110;
+}
+
 #[derive(Debug, Default)]
 pub struct StoreResponse {
-	pub failed_sequence: Vec<InstanceReference>,
+	pub failed_sequence: Vec<FailedInstance>,
 	pub referenced_sequence: Vec<InstanceReference>,
 }
 
+impl StoreResponse {
+	/// The HTTP status code of the response.
+	/// <https://dicom.nema.org/medical/dicom/current/output/chtml/part18/sect_10.5.3.html>
+	pub const fn status_code(&self) -> StatusCode {
+		match (
+			self.referenced_sequence.is_empty(),
+			self.failed_sequence.is_empty(),
+		) {
+			// Some instances failed, others were stored.
+			(false, false) => StatusCode::ACCEPTED,
+			// No instance was stored.
+			(true, false) => StatusCode::CONFLICT,
+			// Every instance was stored (or there was nothing to store).
+			(_, true) => StatusCode::OK,
+		}
+	}
+}
+
 impl From<StoreResponse> for InMemDicomObject {
 	fn from(response: StoreResponse) -> Self {
 		let mut object = Self::new_empty();
@@ -69,12 +104,17 @@ impl From<StoreResponse> for InMemDicomObject {
 				DataElement::new(
 					tags::REFERENCED_SOP_INSTANCE_UID,
 					VR::UI,
-					dicom_value!(Str, failed.sop_instance_uid),
+					dicom_value!(Str, failed.reference.sop_instance_uid),
 				),
 				DataElement::new(
 					tags::REFERENCED_SOP_CLASS_UID,
 					VR::UI,
-					dicom_value!(Str, failed.sop_class_uid),
+					dicom_value!(Str, failed.reference.sop_class_uid),
+				),
+				DataElement::new(
+					tags::FAILURE_REASON,
+					VR::US,
+					dicom_value!(U16, [failed.failure_reason]),
 				),
 			]);
 			failed_items.push(item);
@@ -152,3 +192,63 @@ impl From<multer::Error> for StoreError {
 		Self::Stream(error)
 	}
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+
+	fn reference() -> InstanceReference {
+		InstanceReference {
+			sop_class_uid: UI::from("1.2.840.10008.5.1.4.1.1.2"),
+			sop_instance_uid: UI::from("1.2.3"),
+		}
+	}
+
+	fn failed() -> FailedInstance {
+		FailedInstance {
+			reference: reference(),
+			failure_reason: failure_reason::PROCESSING_FAILURE,
+		}
+	}
+
+	#[test]
+	fn status_code_reflects_stored_and_failed_instances() {
+		let response = |referenced_sequence, failed_sequence| StoreResponse {
+			failed_sequence,
+			referenced_sequence,
+		};
+
+		assert_eq!(
+			response(vec![reference()], vec![]).status_code(),
+			StatusCode::OK
+		);
+		assert_eq!(
+			response(vec![reference()], vec![failed()]).status_code(),
+			StatusCode::ACCEPTED
+		);
+		assert_eq!(
+			response(vec![], vec![failed()]).status_code(),
+			StatusCode::CONFLICT
+		);
+		assert_eq!(response(vec![], vec![]).status_code(), StatusCode::OK);
+	}
+
+	#[test]
+	fn failed_sequence_contains_failure_reason() {
+		let object = InMemDicomObject::from(StoreResponse {
+			failed_sequence: vec![failed()],
+			referenced_sequence: vec![],
+		});
+
+		let items = object
+			.element(tags::FAILED_SOP_SEQUENCE)
+			.unwrap()
+			.items()
+			.unwrap();
+		let failure_reason = items[0]
+			.element(tags::FAILURE_REASON)
+			.unwrap()
+			.to_int::<US>();
+		assert_eq!(failure_reason.unwrap(), failure_reason::PROCESSING_FAILURE);
+	}
+}

src/backend/dimse/cstore/mod.rs

@@ -1,15 +1,17 @@
 pub mod storescp;
 pub mod storescu;
 
-use crate::backend::dimse::{DicomMessage, DATA_SET_EXISTS, DATA_SET_MISSING};
+use crate::backend::dimse::{DicomMessage, ReadError, DATA_SET_EXISTS, DATA_SET_MISSING};
 use crate::types::{AE, UI, US};
 use dicom::core::{DataElement, VR};
 use dicom::dicom_value;
 use dicom::dictionary_std::tags;
+use dicom::object::mem::InMemElement;
 use dicom::object::InMemDicomObject;
 
 // Magic numbers defined by the DICOM specification.
 pub const COMMAND_FIELD_COMPOSITE_STORE_REQUEST: US = 0x0001;
+pub const STATUS_SUCCESS: US = 0x0000;
 
 /// C-STORE-RQ
 pub struct CompositeStoreRequest {
@@ -55,6 +57,7 @@ pub struct CompositeStoreResponse {
 	pub message_id: US,
 	pub sop_class_uid: UI,
 	pub sop_instance_uid: UI,
+	pub status: US,
 }
 
 impl From<CompositeStoreResponse> for DicomMessage {
@@ -65,7 +68,7 @@ impl From<CompositeStoreResponse> for DicomMessage {
             DataElement::new(tags::COMMAND_FIELD, VR::US, dicom_value!(U16, [0x8001])),
             DataElement::new(tags::MESSAGE_ID_BEING_RESPONDED_TO, VR::US, dicom_value!(U16, [response.message_id])),
             DataElement::new(tags::COMMAND_DATA_SET_TYPE, VR::US, dicom_value!(U16, [DATA_SET_MISSING])),
-            DataElement::new(tags::STATUS, VR::US, dicom_value!(U16, [0x00000])),
+            DataElement::new(tags::STATUS, VR::US, dicom_value!(U16, [response.status])),
             DataElement::new(tags::AFFECTED_SOP_INSTANCE_UID, VR::UI, dicom_value!(Str, response.sop_instance_uid))
         ]);
 
@@ -76,3 +79,38 @@ impl From<CompositeStoreResponse> for DicomMessage {
         }
     }
 }
+
+/// The outcome of a C-STORE operation, as reported in the C-STORE-RSP of the peer.
+#[derive(Debug)]
+pub struct CompositeStoreOutcome {
+	/// Status (0000,0900)
+	pub status: US,
+	/// Error Comment (0000,0902), which the peer may send along a non-success status.
+	pub error_comment: Option<String>,
+}
+
+impl TryFrom<&DicomMessage> for CompositeStoreOutcome {
+	type Error = ReadError;
+
+	fn try_from(message: &DicomMessage) -> Result<Self, Self::Error> {
+		let status = message
+			.command
+			.get(tags::STATUS)
+			.map(InMemElement::to_int::<US>)
+			.and_then(Result::ok)
+			.ok_or(ReadError::MissingAttribute(tags::STATUS))?;
+
+		let error_comment = message
+			.command
+			.get(tags::ERROR_COMMENT)
+			.map(InMemElement::to_str)
+			.and_then(Result::ok)
+			.map(|comment| comment.trim_end_matches([' ', '\0']).to_owned())
+			.filter(|comment| !comment.is_empty());
+
+		Ok(Self {
+			status,
+			error_comment,
+		})
+	}
+}

src/backend/dimse/cstore/storescp.rs

@@ -3,7 +3,7 @@ use crate::backend::dimse::cmove::{
 	MediatorError, MoveMediator, MoveSubOperation, SubscriptionTopic,
 };
 use crate::backend::dimse::cstore::{
-	CompositeStoreResponse, COMMAND_FIELD_COMPOSITE_STORE_REQUEST,
+	CompositeStoreResponse, COMMAND_FIELD_COMPOSITE_STORE_REQUEST, STATUS_SUCCESS,
 };
 use crate::backend::dimse::stgcmt::store::StorageCommitmentStore;
 use crate::backend::dimse::stgcmt::{handle_event_report, COMMAND_FIELD_N_EVENT_REPORT_REQUEST};
@@ -181,6 +181,7 @@ impl StoreServiceClassProvider {
 			sop_instance_uid: UI::from(sop_instance_uid.clone()),
 			sop_class_uid: UI::from(sop_class_uid.clone()),
 			message_id,
+			status: STATUS_SUCCESS,
 		};
 
 		let move_originator_id = message

src/backend/dimse/cstore/storescu.rs

@@ -1,15 +1,16 @@
+use crate::api::stow::failure_reason;
 use crate::backend::dimse::association;
-use crate::backend::dimse::cstore::CompositeStoreRequest;
+use crate::backend::dimse::cstore::{CompositeStoreOutcome, CompositeStoreRequest};
 use crate::backend::dimse::{
-	next_message_id, DicomMessageReader, DicomMessageWriter, ReadError, WriteError,
+	next_message_id, DicomMessageReader, DicomMessageWriter, ReadError, StatusType, WriteError,
 };
 use crate::types::{Priority, UI, US};
 use association::pool::{AssociationPool, PoolError, PresentationParameter};
 use association::AssociationError;
 use dicom::object::{FileDicomObject, InMemDicomObject};
 use std::time::Duration;
 use thiserror::Error;
-use tracing::trace;
+use tracing::{trace, warn};
 
 pub struct StoreServiceClassUser {
 	pool: AssociationPool,
@@ -31,6 +32,7 @@ impl StoreServiceClassUser {
 			})
 			.await?;
 
+		let sop_instance_uid = file.meta().media_storage_sop_instance_uid().to_owned();
 		let request = CompositeStoreRequest {
 			affected_sop_class_uid: file.meta().media_storage_sop_class_uid.clone(),
 			affected_sop_instance_uid: file.meta().media_storage_sop_instance_uid.clone(),
@@ -46,10 +48,30 @@ impl StoreServiceClassUser {
 		trace!("Sent C-STORE-RQ");
 
 		let read = association.read_message(self.timeout).await;
-		association.discard_on_err(read)?;
+		let response = association.discard_on_err(read)?;
 		trace!("Received C-STORE-RSP");
 
-		Ok(())
+		// A non-success status is a regular DIMSE response, so the association stays usable.
+		let outcome = CompositeStoreOutcome::try_from(&response)?;
+		match StatusType::try_from(outcome.status) {
+			Ok(StatusType::Success) => Ok(()),
+			// PS3.4 B.2.3: the instance was stored, but e.g. some elements were coerced.
+			Ok(StatusType::Warning) => {
+				warn!(
+					sop_instance_uid,
+					status = format!("{:#06X}", outcome.status),
+					error_comment = outcome.error_comment,
+					"C-STORE completed with a warning"
+				);
+				Ok(())
+			}
+			// Failure, but also statuses that are not valid for C-STORE (Pending, Cancel)
+			// or unknown to us (e.g. the 0xCxxx "Cannot understand" range).
+			_ => Err(StoreError::Status {
+				status: outcome.status,
+				error_comment: outcome.error_comment,
+			}),
+		}
 	}
 }
 
@@ -61,4 +83,27 @@ pub enum StoreError {
 	Write(#[from] WriteError),
 	#[error(transparent)]
 	Association(#[from] PoolError<AssociationError>),
+	#[error(
+		"C-STORE failed with status {status:#06X}: {}",
+		.error_comment.as_deref().unwrap_or("no error comment")
+	)]
+	Status {
+		status: US,
+		error_comment: Option<String>,
+	},
+}
+
+impl StoreError {
+	/// The Failure Reason (0008,1197) to report to the STOW-RS client.
+	pub const fn failure_reason(&self) -> US {
+		match self {
+			// The C-STORE status codes are valid failure reasons (PS3.18 Annex I).
+			Self::Status { status, .. } => *status,
+			// Includes an association that was rejected, e.g. because the peer does not support
+			// the SOP class or transfer syntax. The reason for the rejection is not available.
+			Self::Read(_) | Self::Write(_) | Self::Association(_) => {
+				failure_reason::PROCESSING_FAILURE
+			}
+		}
+	}
 }

src/backend/dimse/stow.rs

@@ -1,12 +1,14 @@
-use crate::api::stow::{InstanceReference, StoreError, StoreRequest, StoreResponse, StowService};
+use crate::api::stow::{
+	FailedInstance, InstanceReference, StoreError, StoreRequest, StoreResponse, StowService,
+};
 use crate::backend::dimse::association;
 use crate::backend::dimse::cstore::storescu::StoreServiceClassUser;
 use crate::types::UI;
 use association::pool::AssociationPool;
 use async_trait::async_trait;
 use futures::TryStreamExt;
 use std::time::Duration;
-use tracing::info;
+use tracing::{info, warn};
 
 pub struct DimseStowService {
 	storescu: StoreServiceClassUser,
@@ -40,10 +42,13 @@ impl StowService for DimseStowService {
 					});
 				}
 				Err(err) => {
-					info!(sop_instance_uid, "Failed to store instance: {err}",);
-					failed_sequence.push(InstanceReference {
-						sop_class_uid,
-						sop_instance_uid,
+					warn!(sop_instance_uid, "Failed to store instance: {err}");
+					failed_sequence.push(FailedInstance {
+						reference: InstanceReference {
+							sop_class_uid,
+							sop_instance_uid,
+						},
+						failure_reason: err.failure_reason(),
 					});
 				}
 			}