Skip to content

Honour errorStrategy for container resolution failures - #7542

Open
Mohit-Ak wants to merge 2 commits into
nextflow-io:masterfrom
Mohit-Ak:fix/container-resolver-error-strategy
Open

Mohit-Ak wants to merge 2 commits into
nextflow-io:masterfrom
Mohit-Ak:fix/container-resolver-error-strategy

Conversation

@Mohit-Ak

Copy link
Copy Markdown
Contributor

Container resolution runs during task hash computation, on a dataflow operator thread, well before the task ever reaches the executor. When a ContainerResolver fails there with a plain unchecked exception, the error arrives at TaskProcessor.resumeOrDie() unwrapped — and the guard there is:

if( task && error instanceof ProcessException ) {
    ...
    errorStrategy = checkErrorStrategy(task, error, taskErrCount, procErrCount, submitRetries)

nf-wave's BadResponseException extends RuntimeException, not ProcessException, so the guard fails, checkErrorStrategy() is never entered, and errorStrategy keeps its initial value of TERMINATE. The configured strategy is not consulted at all, and nothing is logged to say so.

The user-visible consequence is that finish is silently downgraded to terminate. That distinction matters: finish says nothing about the failed task, it governs what happens to everything else already running. With terminate, unrelated in-flight jobs get killed because one process had an unreachable image.

Worth noting the same plugin already gets this right on its asynchronous path — WaveClient.checkContainerCompletion() throws ProcessUnrecoverableException, which is a ProcessException, so it reaches checkErrorStrategy() and behaves correctly. Two code paths, the same logical failure, different outcomes decided by which exception class happens to be thrown.

What this changes

I fixed it in core rather than in nf-wave, at the single point where any resolver is invoked — TaskRun.containerInfo0(). That covers DefaultContainerResolver, WaveContainerResolver, and any third-party plugin resolver, rather than patching one throw site in one plugin and leaving the same shape live elsewhere.

resolveImage0() wraps the resolver call and normalises failures to ProcessUnrecoverableException, which is exactly the classification this failure deserves. checkErrorStrategy() then does the rest with no further changes:

if( error instanceof ProcessUnrecoverableException || error.cause instanceof ProcessUnrecoverableException ) {
    return !action.soft ? action : TERMINATE
}

retry and ignore are soft, so they still escalate to TERMINATE — unchanged, and deliberately so; retrying an unresolvable image is pointless. finish and terminate are hard, so they pass through as configured. That is the whole fix.

Two details in the implementation:

  • Errors that already carry process semantics (ProcessException and subclasses) are rethrown untouched, so the async Wave path and anything else already doing the right thing is unaffected.
  • A checked exception thrown through a proxied resolver surfaces as UndeclaredThrowableException; that gets unwrapped first so the real cause is what's classified and reported. I hit this while writing the tests, not in theory.

I did not widen the resumeOrDie() guard itself (option 2 in the issue) — it has a much larger blast radius over every error path in the processor, and the resolver call site gets the same coverage for the failure class actually at hand.

Testing

Three cases added to TaskRunTest, covering the reported failure and the two boundaries around it: an unchecked resolver failure is wrapped, an already-ProcessException failure is not re-wrapped, and a java.lang.Error still propagates untouched rather than being swallowed into a process exception.

RED check first — with the tests in place and the fix stashed:

TaskRunTest > should wrap container resolution errors as unrecoverable process errors FAILED
    Expected exception of type 'nextflow.exception.ProcessUnrecoverableException',
    but got 'java.lang.RuntimeException'
    Caused by: java.lang.RuntimeException: Wave invalid response: POST /v1alpha2/container [400]

TaskRunTest > should not wrap container resolution errors that are already process errors FAILED
    Expected exception of type 'nextflow.exception.ProcessUnrecoverableException',
    but got 'java.lang.reflect.UndeclaredThrowableException'

That first failure is the bug: the plain RuntimeException escapes unclassified, which is precisely what makes resumeOrDie() skip the strategy check.

Then the full module suite, ./gradlew :nextflow:test:

  • pristine master (2e13110): 4217 tests, 0 failures, 0 errors, 89 skipped
  • with this change: 4220 tests, 0 failures, 0 errors, 89 skipped

Delta is exactly the three added tests, so nothing was broken, silently skipped, or deselected.

What I could not verify end-to-end is the reproducer from the issue, since it needs a live Wave endpoint and an AWS Batch executor. The unit tests pin the exception classification, which is the actual defect; the behavioural consequence for finish follows from checkErrorStrategy(), which is unchanged and already covered by the existing suite.

Fixes #7446

Container images are resolved during task hash computation, before the task
reaches the executor. A ContainerResolver that fails with a plain unchecked
exception (e.g. nf-wave's BadResponseException, which extends RuntimeException)
reaches TaskProcessor.resumeOrDie unwrapped, where the
'error instanceof ProcessException' guard rejects it. checkErrorStrategy() is
never entered, so the configured errorStrategy is not consulted at all and
'finish' is silently downgraded to 'terminate', killing in-flight tasks.

Normalise resolver failures to ProcessUnrecoverableException at the single
call site in TaskRun so the usual classification runs: 'retry' and 'ignore'
are still escalated to 'terminate' because the failure is unrecoverable, while
'finish' is honoured. Failures that already carry process semantics pass
through untouched, and a checked exception rethrown by a proxied resolver as
UndeclaredThrowableException is unwrapped first.

Fixes nextflow-io#7446

Signed-off-by: Mohit Ak <Mohit-Ak@users.noreply.github.com>
@netlify

netlify Bot commented Aug 26, 2026

Copy link
Copy Markdown

Deploy Preview for nextflow-docs canceled.

Name Link
🔨 Latest commit 22dbfc2
🔍 Latest deploy log https://app.netlify.com/projects/nextflow-docs/deploys/6aa57cbcb3e980000822371f

@Mohit-Ak
Mohit-Ak marked this pull request as ready for review August 28, 2026 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Container resolution errors bypass errorStrategy, silently downgrading finish to terminate

2 participants