From d9dcaabb2012c4d096fc71ae7a8f2a0dda277983 Mon Sep 17 00:00:00 2001 From: Stephan Vedder Date: Tue, 10 Nov 2026 14:19:18 +0100 Subject: [PATCH] refactor(stow): handle storage outcome correctly --- deny.toml | 1 + src/api/stgcmt/service.rs | 10 +-- src/api/stow/routes.rs | 3 +- src/api/stow/service.rs | 108 ++++++++++++++++++++++++++- src/backend/dimse/cstore/mod.rs | 42 ++++++++++- src/backend/dimse/cstore/storescp.rs | 3 +- src/backend/dimse/cstore/storescu.rs | 55 ++++++++++++-- src/backend/dimse/stow.rs | 17 +++-- tests/stow.rs | 76 ++++++++++++++++--- 9 files changed, 278 insertions(+), 37 deletions(-) diff --git a/deny.toml b/deny.toml index 81390d2..51b1868 100644 --- a/deny.toml +++ b/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] diff --git a/src/api/stgcmt/service.rs b/src/api/stgcmt/service.rs index 8bd56d8..6644927 100644 --- a/src/api/stgcmt/service.rs +++ b/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. /// diff --git a/src/api/stow/routes.rs b/src/api/stow/routes.rs index 57cdd68..7b02baf 100644 --- a/src/api/stow/routes.rs +++ b/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()) diff --git a/src/api/stow/service.rs b/src/api/stow/service.rs index 7f2044c..c3a79fe 100644 --- a/src/api/stow/service.rs +++ b/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. +/// +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, + pub failed_sequence: Vec, pub referenced_sequence: Vec, } +impl StoreResponse { + /// The HTTP status code of the response. + /// + 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 for InMemDicomObject { fn from(response: StoreResponse) -> Self { let mut object = Self::new_empty(); @@ -69,12 +104,17 @@ impl From 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 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::(); + assert_eq!(failure_reason.unwrap(), failure_reason::PROCESSING_FAILURE); + } +} diff --git a/src/backend/dimse/cstore/mod.rs b/src/backend/dimse/cstore/mod.rs index 9a6e2d8..fc31822 100644 --- a/src/backend/dimse/cstore/mod.rs +++ b/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 for DicomMessage { @@ -65,7 +68,7 @@ impl From 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 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, +} + +impl TryFrom<&DicomMessage> for CompositeStoreOutcome { + type Error = ReadError; + + fn try_from(message: &DicomMessage) -> Result { + let status = message + .command + .get(tags::STATUS) + .map(InMemElement::to_int::) + .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, + }) + } +} diff --git a/src/backend/dimse/cstore/storescp.rs b/src/backend/dimse/cstore/storescp.rs index ff9e642..d7656aa 100644 --- a/src/backend/dimse/cstore/storescp.rs +++ b/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 diff --git a/src/backend/dimse/cstore/storescu.rs b/src/backend/dimse/cstore/storescu.rs index 3224f63..8e22752 100644 --- a/src/backend/dimse/cstore/storescu.rs +++ b/src/backend/dimse/cstore/storescu.rs @@ -1,7 +1,8 @@ +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}; @@ -9,7 +10,7 @@ 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), + #[error( + "C-STORE failed with status {status:#06X}: {}", + .error_comment.as_deref().unwrap_or("no error comment") + )] + Status { + status: US, + error_comment: Option, + }, +} + +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 + } + } + } } diff --git a/src/backend/dimse/stow.rs b/src/backend/dimse/stow.rs index 38de105..cde23ba 100644 --- a/src/backend/dimse/stow.rs +++ b/src/backend/dimse/stow.rs @@ -1,4 +1,6 @@ -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; @@ -6,7 +8,7 @@ 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(), }); } } diff --git a/tests/stow.rs b/tests/stow.rs index 6c4fcfb..0ac20ad 100644 --- a/tests/stow.rs +++ b/tests/stow.rs @@ -105,12 +105,11 @@ async fn does_not_leak_semaphore_permits_if_association_is_rejected() -> anyhow: with_test_environment(config, async |client| { let start = Instant::now(); - let response = client + let result = client .store_instances() .with_instances(futures::stream::iter(instances)) .run() - .await - .context("STOW-RS request failed")?; + .await; let elapsed = start.elapsed(); assert!( @@ -118,14 +117,15 @@ async fn does_not_leak_semaphore_permits_if_association_is_rejected() -> anyhow: "STOW-RS took {elapsed:?}; expected fast failure" ); - let failed_sequence = response - .element(tags::FAILED_SOP_SEQUENCE) - .context("STOW-RS response is missing FailedSOPSequence")?; + // No instance was stored, so the request fails as a whole. assert!( - failed_sequence - .items() - .is_some_and(|items| items.len() == 3), - "All three rejected instances should appear in FailedSOPSequence" + matches!( + result, + Err(DicomWebError::HttpStatusFailure { + status_code: StatusCode::CONFLICT + }) + ), + "Expected 409 Conflict, got {result:?}" ); Ok(()) @@ -133,6 +133,62 @@ async fn does_not_leak_semaphore_permits_if_association_is_rejected() -> anyhow: .await } +#[tokio::test] +async fn returns_failure_reason_if_some_instances_fail() -> anyhow::Result<()> { + let config = " + server: + http: + port: 0 + dimse: + - aet: DICOM-RST + interface: 0.0.0.0 + port: 0 + aets: + - aet: ORTHANC + host: 127.0.0.1 + port: ${ORTHANC_PORT} + backend: DIMSE + "; + + let valid = open_file(dicom_test_files::path("pydicom/CT_small.dcm").unwrap()).unwrap(); + let mut invalid = valid.clone(); + // Set a fake SOPClassUID; Orthanc will reject the association for this instance + invalid.meta_mut().media_storage_sop_class_uid = String::from("1.2.3.4.5.6.7.8.9.10"); + + with_test_environment(config, async |client| { + let response = client + .store_instances() + .with_instances(futures::stream::iter([valid, invalid])) + .run() + .await + .context("STOW-RS request failed")?; + + let referenced_items = response + .element(tags::REFERENCED_SOP_SEQUENCE) + .context("STOW-RS response is missing ReferencedSOPSequence")? + .items() + .context("ReferencedSOPSequence is not a sequence")?; + assert_eq!(referenced_items.len(), 1); + + let failed_items = response + .element(tags::FAILED_SOP_SEQUENCE) + .context("STOW-RS response is missing FailedSOPSequence")? + .items() + .context("FailedSOPSequence is not a sequence")?; + assert_eq!(failed_items.len(), 1); + + let failure_reason = failed_items[0] + .element(tags::FAILURE_REASON) + .context("FailedSOPSequence item is missing FailureReason")? + .to_int::()?; + // Processing failure, as the reason for the rejected association is not known + assert_eq!(failure_reason, 0x0110); + + Ok(()) + }) + .await +} + // https://github.com/UMEssen/DICOM-RST/issues/55 #[tokio::test] async fn returns_413_if_max_upload_size_is_exceeded() -> anyhow::Result<()> {