From 166098a6da5bd170aa42f08977c8784225507e04 Mon Sep 17 00:00:00 2001 From: Ben Sherman Date: Mon, 13 Jul 2026 16:39:25 -0500 Subject: [PATCH 1/2] Fix ConcurrentModificationException with container array jobs #6108 TaskArrayRun.getContainerConfig() appended the executor array-index variable (e.g. SLURM_ARRAY_TASK_ID) to the container env whitelist by mutating the list in place. Because ContainerHelper.parseEnvWhitelist() returns the session config list by reference, that list is shared across every ContainerConfig rebuilt from the session config, so one actor thread mutating it while another iterated it in BashWrapperBuilder/FusionHelper threw a java.util.ConcurrentModificationException (and the whitelist grew unboundedly). Stop mutating the shared container config. The array index variable is already carried on TaskBean, so instead add it to the container environment where the whitelist is applied: BashWrapperBuilder.createContainerBuilder() for the standard container path and FusionHelper.runWithContainer() for the Fusion path (reading it from the launcher's TaskBean). The shared session config is never modified. Signed-off-by: Ben Sherman Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Ben Sherman --- .../executor/BashWrapperBuilder.groovy | 5 ++++ .../nextflow/fusion/FusionHelper.groovy | 5 ++++ .../nextflow/processor/TaskArrayRun.groovy | 13 --------- .../executor/BashWrapperBuilderTest.groovy | 29 +++++++++++++++++++ .../nextflow/fusion/FusionHelperTest.groovy | 14 +++++++++ .../processor/TaskArrayCollectorTest.groovy | 3 +- 6 files changed, 55 insertions(+), 14 deletions(-) diff --git a/modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy b/modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy index d47f7b7368..f16d38ab45 100644 --- a/modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy +++ b/modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy @@ -742,6 +742,11 @@ class BashWrapperBuilder { builder.addEnv(var) } + // for job arrays, propagate the array index variable (e.g. SLURM_ARRAY_TASK_ID) + // into the container environment + if( arrayIndexName ) + builder.addEnv(arrayIndexName) + // when secret are not managed by the execution platform natively // the secret names are added to the container env var white list if( !isSecretNative() && secretNames ) { diff --git a/modules/nextflow/src/main/groovy/nextflow/fusion/FusionHelper.groovy b/modules/nextflow/src/main/groovy/nextflow/fusion/FusionHelper.groovy index b0adf1703f..5af326c5ea 100644 --- a/modules/nextflow/src/main/groovy/nextflow/fusion/FusionHelper.groovy +++ b/modules/nextflow/src/main/groovy/nextflow/fusion/FusionHelper.groovy @@ -63,6 +63,11 @@ class FusionHelper { for( String env : containerConfig.getEnvWhitelist()) containerBuilder.addEnv(env) + // for job arrays, propagate the array index variable (e.g. SLURM_ARRAY_TASK_ID) + // into the container environment + if( launcher.arrayIndexName ) + containerBuilder.addEnv(launcher.arrayIndexName) + // patch the cmd wrapping the last item in quotes final patchCmd = new ArrayList(runCmd) patchCmd[-1] = "'${patchCmd[-1]}'" diff --git a/modules/nextflow/src/main/groovy/nextflow/processor/TaskArrayRun.groovy b/modules/nextflow/src/main/groovy/nextflow/processor/TaskArrayRun.groovy index c68eeea6d0..1ef331fec9 100644 --- a/modules/nextflow/src/main/groovy/nextflow/processor/TaskArrayRun.groovy +++ b/modules/nextflow/src/main/groovy/nextflow/processor/TaskArrayRun.groovy @@ -18,8 +18,6 @@ package nextflow.processor import groovy.transform.CompileStatic import groovy.util.logging.Slf4j -import nextflow.container.ContainerConfig -import nextflow.executor.TaskArrayExecutor /** * Models a task array, which submits a collection of independent @@ -37,17 +35,6 @@ class TaskArrayRun extends TaskRun { children.size() } - @Override - ContainerConfig getContainerConfig() { - final config = super.getContainerConfig() - final envWhitelist = config.getEnvWhitelist() - if( envWhitelist != null ) { - final executor = (TaskArrayExecutor)processor.getExecutor() - envWhitelist.add(executor.getArrayIndexName()) - } - return config - } - @Override boolean isContainerEnabled() { return false diff --git a/modules/nextflow/src/test/groovy/nextflow/executor/BashWrapperBuilderTest.groovy b/modules/nextflow/src/test/groovy/nextflow/executor/BashWrapperBuilderTest.groovy index 75e1c1c99f..b110b6e0ef 100644 --- a/modules/nextflow/src/test/groovy/nextflow/executor/BashWrapperBuilderTest.groovy +++ b/modules/nextflow/src/test/groovy/nextflow/executor/BashWrapperBuilderTest.groovy @@ -227,6 +227,35 @@ class BashWrapperBuilderTest extends Specification { builder.mounts == [ Paths.get('/my/bin') ] } + def 'should add the array index variable to the container env' () { + given: + def bash = Spy(new BashWrapperBuilder(Mock(TaskBean))) + and: + bash.getEnvironment() >> [:] + bash.getBinDirs() >> [] + bash.getWorkDir() >> Paths.get('/my/work/dir') + bash.isStatsEnabled() >> false + bash.getStageInMode() >> 'symlink' + bash.getInputFiles() >> [:] + bash.getContainerConfig() >> new SingularityConfig(envWhitelist: 'FOO,BAR') + bash.getContainerImage() >> 'foo/bar' + bash.getContainerMount() >> null + bash.getContainerMemory() >> null + bash.getContainerCpus() >> null + bash.getContainerCpuset() >> null + bash.getContainerOptions() >> null + bash.getContainerPlatform() >> 'amd64' + bash.isSecretNative() >> false + bash.getSecretNames() >> [] + and: 'the task is an array child, exposing the array index variable' + bash.getArrayIndexName() >> 'SLURM_ARRAY_TASK_ID' + + when: + def builder = bash.createContainerBuilder(null) + then: 'the array index variable is whitelisted without mutating the container config' + builder.env == ['NXF_TASK_WORKDIR', 'FOO', 'BAR', 'SLURM_ARRAY_TASK_ID'] + } + def 'should add resolved inputs'() { given: def bash = Spy(new BashWrapperBuilder(Mock(TaskBean))) diff --git a/modules/nextflow/src/test/groovy/nextflow/fusion/FusionHelperTest.groovy b/modules/nextflow/src/test/groovy/nextflow/fusion/FusionHelperTest.groovy index 75632d8eb5..a99c9f1e7d 100644 --- a/modules/nextflow/src/test/groovy/nextflow/fusion/FusionHelperTest.groovy +++ b/modules/nextflow/src/test/groovy/nextflow/fusion/FusionHelperTest.groovy @@ -60,6 +60,20 @@ class FusionHelperTest extends Specification { result == "docker run -i --platform linux/amd64 --rm --privileged image:1 echo 'hello'" } + def 'should propagate the array index variable to the fusion container' () { + given: + def launcher = Mock(FusionScriptLauncher) { + fusionEnv() >> [:] + getArrayIndexName() >> 'SLURM_ARRAY_TASK_ID' + } + def config = new DockerConfig([:]) + + when: + def result = FusionHelper.runWithContainer(launcher, config, 'image:1', null, ['echo', 'hello']) + then: + result.contains('-e "SLURM_ARRAY_TASK_ID"') + } + def 'should return fusion container command' () { given: def launcher = Mock(FusionScriptLauncher) { diff --git a/modules/nextflow/src/test/groovy/nextflow/processor/TaskArrayCollectorTest.groovy b/modules/nextflow/src/test/groovy/nextflow/processor/TaskArrayCollectorTest.groovy index 989a581134..fe3b042525 100644 --- a/modules/nextflow/src/test/groovy/nextflow/processor/TaskArrayCollectorTest.groovy +++ b/modules/nextflow/src/test/groovy/nextflow/processor/TaskArrayCollectorTest.groovy @@ -132,7 +132,8 @@ class TaskArrayCollectorTest extends Specification { taskArray.script == 'the-task-array-script' and: taskArray.getArraySize() == 3 - taskArray.getContainerConfig().getEnvWhitelist() == [ 'ARRAY_JOB_INDEX' ] + and: 'the array index variable is no longer injected into the shared container config; it is added to the container env by the launcher (BashWrapperBuilder/FusionHelper)' + taskArray.getContainerConfig().getEnvWhitelist() == [] taskArray.isContainerEnabled() == false } From 21aef4fdf27b614dc88b9036fbb1f93a8c82c760 Mon Sep 17 00:00:00 2001 From: jorgee Date: Tue, 14 Jul 2026 13:58:54 +0200 Subject: [PATCH 2/2] Propagate array index variable to containerised grid array children Removing the shared-config mutation (previous commit) left the array index variable (e.g. SLURM_ARRAY_TASK_ID) no longer reaching the container for non-Fusion grid job arrays: for those executors the container is built from each child TaskRun, whose TaskBean.arrayIndexName is null (only a TaskArrayRun populates it), so the whitelist branch added in BashWrapperBuilder never fired. Inject the executor array index name onto the child launcher in GridTaskHandler.createTaskWrapper() when the handler is an array child, so the already-added BashWrapperBuilder branch exposes it in the container env. Guard the `### array:` metadata block on arrayWorkDirs so array children (which carry only the index name, not the work-dirs) don't render a bogus block / NPE. Add GridTaskHandlerTest cases exercising the real child path (a plain child builder with a null arrayIndexName that the handler must populate), which fail without the fix. Signed-off-by: Jorge Ejarque Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: jorgee --- .../executor/BashWrapperBuilder.groovy | 4 +- .../nextflow/executor/GridTaskHandler.groovy | 12 +++-- .../executor/GridTaskHandlerTest.groovy | 45 +++++++++++++++++++ 3 files changed, 57 insertions(+), 4 deletions(-) diff --git a/modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy b/modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy index f16d38ab45..74095b5d09 100644 --- a/modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy +++ b/modules/nextflow/src/main/groovy/nextflow/executor/BashWrapperBuilder.groovy @@ -504,7 +504,9 @@ class BashWrapperBuilder { final lines = new StringBuilder() lines << '### ---\n' lines << "### name: '${bean.name}'\n" - if( bean.arrayIndexName ) { + // the array metadata block only applies to the array dispatcher task (which carries the + // child work-dirs); array children only carry the index name to expose it in the container + if( bean.arrayIndexName && bean.arrayWorkDirs ) { lines << '### array:\n' lines << "### index-name: ${bean.arrayIndexName}\n" lines << "### index-start: ${bean.arrayIndexStart}\n" diff --git a/modules/nextflow/src/main/groovy/nextflow/executor/GridTaskHandler.groovy b/modules/nextflow/src/main/groovy/nextflow/executor/GridTaskHandler.groovy index 7a5f8ca572..1462ce63b6 100644 --- a/modules/nextflow/src/main/groovy/nextflow/executor/GridTaskHandler.groovy +++ b/modules/nextflow/src/main/groovy/nextflow/executor/GridTaskHandler.groovy @@ -209,9 +209,15 @@ class GridTaskHandler extends TaskHandler implements FusionAwareTask { } protected BashWrapperBuilder createTaskWrapper(TaskRun task) { - return fusionEnabled() - ? fusionLauncher() - : executor.createBashWrapperBuilder(task) + if( fusionEnabled() ) + return fusionLauncher() + final builder = executor.createBashWrapperBuilder(task) + // for a containerised array child, the container is built from the child task bean + // (which does not carry the array index variable); expose the scheduler array-index + // variable (e.g. SLURM_ARRAY_TASK_ID) inside the task container + if( isArrayChild && executor instanceof TaskArrayExecutor ) + builder.arrayIndexName = ((TaskArrayExecutor) executor).getArrayIndexName() + return builder } protected String stdinLauncherScript() { diff --git a/modules/nextflow/src/test/groovy/nextflow/executor/GridTaskHandlerTest.groovy b/modules/nextflow/src/test/groovy/nextflow/executor/GridTaskHandlerTest.groovy index 48303aa5d3..d6c2d821f7 100644 --- a/modules/nextflow/src/test/groovy/nextflow/executor/GridTaskHandlerTest.groovy +++ b/modules/nextflow/src/test/groovy/nextflow/executor/GridTaskHandlerTest.groovy @@ -148,4 +148,49 @@ class GridTaskHandlerTest extends Specification { LAUNCH_COMMAND_EOF '''.stripIndent() } + + def 'should propagate the array index variable to a containerised array child launcher' () { + given: 'a plain child builder built from a non-array TaskRun (arrayIndexName not set - the problematic state)' + def childBuilder = new BashWrapperBuilder(new TaskBean(name: 'foo', workDir: Paths.get('/work/dir'), inputFiles: [:])) + and: + def task = Mock(TaskRun) { getWorkDir() >> Paths.get('/work/dir') } + def exec = Mock(SlurmExecutor) { + getConfig() >> new ExecutorConfig([:]) + getName() >> 'slurm' + getArrayIndexName() >> 'SLURM_ARRAY_TASK_ID' + createBashWrapperBuilder(task) >> childBuilder + } + def handler = Spy(new GridTaskHandler(task, exec)) + handler.withArrayChild(true) + handler.fusionEnabled() >> false + + expect: 'the child bean does not carry the array index variable on its own' + childBuilder.arrayIndexName == null + + when: + def builder = handler.createTaskWrapper(task) + then: 'the handler injects the executor array index name so the container env can expose it' + builder.arrayIndexName == 'SLURM_ARRAY_TASK_ID' + } + + def 'should not add the array index variable for a non-array task' () { + given: + def childBuilder = new BashWrapperBuilder(new TaskBean(name: 'foo', workDir: Paths.get('/work/dir'), inputFiles: [:])) + and: + def task = Mock(TaskRun) { getWorkDir() >> Paths.get('/work/dir') } + def exec = Mock(SlurmExecutor) { + getConfig() >> new ExecutorConfig([:]) + getName() >> 'slurm' + createBashWrapperBuilder(task) >> childBuilder + } + def handler = Spy(new GridTaskHandler(task, exec)) // isArrayChild == false + handler.fusionEnabled() >> false + + when: + def builder = handler.createTaskWrapper(task) + then: + builder.arrayIndexName == null + and: 'the executor array index name is never requested for a non-array task' + 0 * exec.getArrayIndexName() + } }