Fixes/plugins 362 - #8
Draft
rherrick wants to merge 11 commits into
Draft
Conversation
rherrick
requested review from
bin-xnatworks,
dmaffitt,
ian-xnatworks,
karchie,
kathrynalpert,
mrjamesdickson,
ransfordj,
willhorton-xw and
zachxw
August 24, 2026 15:42
karchie
reviewed
Aug 24, 2026
| throw toCMoveFailure(rspHandler); | ||
| } | ||
| log.debug("C-Move -- Completed: {}", rspHandler.getCompleted()); | ||
| } catch (NoPresentationContextException e) { |
There was a problem hiding this comment.
this is probably uniformly an improvement but I'm slightly concerned that a PACS might reject a presentation context negotiation because of an internal failure. no I don't have any evidence this can actually happen
karchie
reviewed
Aug 24, 2026
| }); | ||
|
|
||
| if (!scanTypes.length) { | ||
| if (!wholeStudy && !scanTypes.length) { |
There was a problem hiding this comment.
no change needed but first a lazy q: is scanTypes.length always a number, so that this is a zero comparison? the fact that I have to ask this makes me prefer an explicit scanTypes.length === 0.
karchie
reviewed
Aug 24, 2026
| data.seriesIds = spawn('pre|style=margin:0', data.seriesIds.join(',\n')).outerHTML | ||
| // A study-level request carries no series list: the whole study was asked | ||
| // for, and which series that is isn't known until the data arrives. | ||
| data.seriesIds = data.seriesIds.length |
There was a problem hiding this comment.
no change needed; again the zero punning makes me nervous. unless sometimes length is null, which might be worse
There was a problem hiding this comment.
this is me being inconsistent because I have no problem with nil punning in Lisp. I think it's that JS syntactically looks like it should be a typed language even though it's the loosest language out there, and so stylistically it feels like the dev should bend over backwards to clarify intent. but the Google JS style guide is silent about this (while telling users to use TS instead)
karchie
approved these changes
Aug 24, 2026
…ures Groundwork for study-level retrieve. A C-MOVE that pulls an entire study runs considerably longer than one pulling a single series, and fails differently, so both the timeouts and the failure handling had to change before study-level retrieve could be viable. Timeouts: the four values hard-coded in QrClient are now supplied by the caller as DimseTimeouts, resolved by DimsePacsClientService from four new preferences (dimseResponseTimeoutSeconds, dimseRetrieveTimeoutSeconds, dimseAcceptTimeoutSeconds, dimseConnectTimeoutSeconds). The defaults reproduce the previous values, so behavior is unchanged until an administrator raises them. They're read per operation, so a change takes effect on the next request rather than at restart. Failure classification: QrClient.move collapsed every failure status into a single PacsQueryException, which RetryablePacsOperation then retried until the request's attempts were spent, whatever the PACS had actually said. Failures a PACS reports because it rejected the request as formed now raise PacsRetrieveNotSupportedException, other permanent failures raise PacsPermanentFailureException, and RetryablePacsOperation fails fast on both. This is also the hook a STUDY-to-SERIES fallback will need. Two defects found along the way: * NoPresentationContextException, which Association.cmove throws when the PACS accepts the association but not a presentation context for the retrieve, extends IOException and was being reported as a lost connection and retried. It's a refusal of the operation, and now reports as one. * QrClientRspHandler masked every status with 0xFF00 before recording it, so 0xA701 and 0xA702 were both stored as 0xA700 and no specific failure could be identified. It now records the full status while still categorizing on the high byte. Status codes appear in messages as hex rather than decimal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…erride Adds the RetrieveLevel enum (STUDY or SERIES) and threads it from configuration through to the queued request. Nothing acts on it yet; the study-level C-MOVE itself follows. SERIES remains the default everywhere, so behavior is unchanged until a PACS or a request asks for STUDY. An administrator sets the level per PACS, and a single import can override it. Resolution — request override, then PACS setting, then SERIES — lives in RetrieveLevel.resolve so the queueing and execution paths can't disagree about what a request asked for. The resolved level is stored on the request rather than recomputed at execution time. A queued request can sit for hours, and the PACS configuration can change in the meantime; storing it means a queued request behaves the way it did when it was queued, and the request history records what was actually done. Both request tables read a missing value back as SERIES, so requests queued before this column existed are unaffected. A study that names the series to import is forced back to SERIES regardless of the configured level, since a study-level retrieve pulls the whole study and can't filter it. This is logged when it happens rather than silently applied. DICOMweb PACS configurations are rejected at validation if set to STUDY: study level is a C-MOVE feature, and DICOMweb retrieval here is per series. The admin form only offers the setting for DIMSE connections and drops it from a DICOMweb submission. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds importStudy through the client layer and branches importFromPacsRequest on the level stored with the request: a STUDY request now issues one C-MOVE identifying only the study, instead of one per series each on its own association. The identifier carries no series key, since that would contradict the level and give the PACS something it may reject. The branch reads the stored level and nothing else. Requests that need a subset of a study were already forced to SERIES when they were queued, so a STUDY request reaching this point always wants the whole study, and re-deriving that here would mean the same rule living in two places. DICOMweb retrieval streams one series at a time and has no study-level equivalent, so DicomWebPacsClientService.importStudy refuses with a message naming the PACS and the setting to change. Validation already rejects the combination, so this is a backstop rather than the path an administrator hits. Also stops importSeries wrapping PacsException in DqrException. PacsException is already a DqrException, so the wrapping was unnecessary, and it hid the specific failure: a caller could no longer tell a PACS that refused the retrieve from one that failed to complete it. That distinction is what a study-to-series fallback would branch on. Retry behavior is unaffected either way, since the retry sits inside doCMove, below the wrapping. doCMove becomes protected so tests can capture the keys that would go on the wire without a PACS on the other end of the association. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…retrieve Queueing a study-level import no longer runs the series-level C-FIND that turns a study into a list of series. Skipping it is most of the performance win: for a batch import it removes one association per study from queueing alone, on top of the associations the retrieve itself no longer opens. The expansion was also where the queue and history views got the study's date, study ID, accession number, and patient details -- they were read off the first series in the result. With no expansion there is no first series, so a study-level queueing now runs a single study-level query instead, which both confirms the study is on the PACS and supplies those fields. A study the PACS doesn't have is still not queued, same as before. The study-level query returns the study date as a parsed Date, where the series query returned the raw DICOM string. Formatting it back to yyyyMMdd keeps stored requests consistent regardless of which path queued them, via a new DqrDateRange.formatDicomDate overload. queueStudyImport is split into queueWholeStudyImport and queueSeriesImport rather than branching inside one method; the two paths share almost nothing beyond the request they produce. Both CSV import paths get the same treatment, so a PACS configured for study-level retrieve isn't quietly expanded when the import comes from a spreadsheet. The import dialog gains an "Import Entire Studies" button, so the study-level path follows from what the user asked for rather than from an empty series filter. It sends no series and sets the level on the request. The button is hidden for DICOMweb connections, which have no study-level retrieve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A DICOMweb PACS can't be configured for study-level retrieve, so a study-level import against one could only come from the request itself, via a direct call to /xapi/dqr/import. That was accepted and queued, then failed once the request reached the PACS. It now fails immediately with a 400 naming the PACS and saying where study-level retrieve is available, so nothing is queued that can only fail. importFromPacs picks up DataFormatException, which is how the rest of this API reports a request it can't act on. The only caller is the controller, which already declares Exception. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Everything downstream of the retrieve assumed a request carried a list of series. A study-level request doesn't: the whole study was asked for, and which series that turned out to be isn't known until the data arrives. The notification email and the workflow entry both enumerated that list. They now describe the study instead when the request was study level, with a subject line to match. The queue's request detail shows the same rather than an empty block, and picks up the retrieve level automatically since it renders whatever the request serializes. The bigger problem was the throttling. The pause between requests is calculated from how long the previous request took, to hold a PACS at its configured utilization -- fine at a minute per series, but a thirty minute study retrieve at 10% utilization asks for a four and a half hour pause, and the queue thread spends the night doing nothing. The pause is now bounded by a new dqrMaxThrottleSleepSeconds preference, defaulting to 1800. That default is high enough that existing series-level installs will not reach it, so their pacing is unchanged; 0 disables the bound for sites that would rather hold utilization exactly. When the bound applies it is logged at INFO, since utilization will run above the configured figure until the queue catches up. No change was needed for the prearchive rebuild, the PLUGINS-323 zero-file failure check, or UpdateRequestStatusArchiveProcessor: all three key off the study instance UID and project, neither of which a study-level request changes. The new workflow test exercises the zero-file path with an empty series list and confirms it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The dicom/command/cmove package had no production caller. Dcm4cheToolCMoveSCU, CMoveSCUSeriesLevel and CFindSCUSeriesLevelByIdWithCMove were reachable only from TestCfindCmove, and the CFindSCUSpecificLevel hooks that existed to support them (cMoveRequestedOnResults, performCMoveOnResults, reportCMoveTargetNotFound) were dead with the subclass gone -- performCMoveOnResults still threw "C-MOVE not yet implemented for dcm4che3". Every C-MOVE the plugin actually issues goes through DimsePacsClientService and QrClient. Verified nothing outside the package referenced any of it, in Java, Spring config, YAML, properties or the UI. TestCfindCmove verified that an unreachable PACS surfaces as a typed exception, which was worth keeping but was being verified through code that never ran. The new TestQrClient asserts the same thing against the live path, and additionally that the failure stays retryable -- a PACS that can't be reached now may be reachable on the next attempt, unlike one that answers and refuses. Also extracts the notification subject line into a testable method. Testing it through runTask meant static-mocking AdminUtils and TurbineUtils, which the inline mock maker cannot redefine, and stubbing populateVmTemplate meant naming its Velocity Context parameter, which is only on the runtime classpath. The branch is what mattered, so that is what is now tested. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers the new per-PACS retrieve level and per-request override, the four DIMSE timeout preferences, the interpretation of C-MOVE failure statuses, the bound on the utilization pause, and the removal of the unused cmove package. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A series-level request with an empty series list looped zero times, contacted no PACS, and reported success. Nothing in the plugin queues such a request deliberately, and it is exactly how a study-level request whose retrieve level did not survive being queued presents itself: the queueing-time study query runs, no C-MOVE follows, and the request completes as though it worked. It now fails with a message naming the study and pointing at the retrieve_level column. Also logs the resolved level and series count at INFO before branching, so which path a request took is visible without turning on debug logging. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…empty The Data Requested column in the queue and history tables rendered "1 Session with N Scans" from the length of the request's series list. A study-level request lists no series by design, so a successful whole-study import displayed as "1 Session with 0 Scans", which reads as though nothing was requested. It now reads "1 Session, entire study". Phase 4 covered the request detail dialog but missed this summary column, which is the one visible without opening anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r series A PACS set to retrieve whole studies never actually did. The import screen always names the series it has selected -- Import Selected refuses to submit with none ticked -- and any request naming series was downgraded to a series-level retrieve. The only way to reach the study-level path was the Import Entire Studies button, so the per-PACS setting had no effect on the action people actually use. Selecting series and retrieving whole studies are mutually exclusive, so the choice is now removed rather than resolved after the fact. For a PACS set to STUDY the import dialog shows the series descriptions and study counts as before, with every checkbox ticked and disabled and hover text explaining that this PACS retrieves entire studies, and Import Selected is not offered at all. The same rule is enforced server-side, since the UI is not the only caller: an import request that names series for a study-level retrieve is now rejected with a message naming the studies involved, rather than silently retrieving them a different way than asked. Nothing queues such a request today, so this only affects direct API callers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rherrick
force-pushed
the
fixes/plugins-362
branch
from
August 27, 2026 18:38
4ccd10c to
89119f5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.