Skip to content

Fix concurrent modification with job arrays#7332

Open
bentsherman wants to merge 2 commits into
masterfrom
fix-6108-no-config-mutation
Open

Fix concurrent modification with job arrays#7332
bentsherman wants to merge 2 commits into
masterfrom
fix-6108-no-config-mutation

Conversation

@bentsherman

Copy link
Copy Markdown
Member

Fix #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 ConcurrentModificationException

This PR removes the shared container config mutation. 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
  • FusionHelper.runWithContainer() for the Fusion path

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 <ben.sherman@seqera.io>

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman bentsherman requested a review from jorgee July 13, 2026 21:59
@netlify

netlify Bot commented Jul 13, 2026

Copy link
Copy Markdown

Deploy Preview for nextflow-docs ready!

Name Link
🔨 Latest commit 21aef4f
🔍 Latest deploy log https://app.netlify.com/projects/nextflow-docs/deploys/6a562483071b6e000866a35f
😎 Deploy Preview https://deploy-preview-7332--nextflow-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@bentsherman bentsherman changed the title Fix concurrent modification with job arrays #6108 Fix concurrent modification with job arrays Jul 13, 2026
@jorgee

jorgee commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Reviewed this before approving and found a gap that needs to be closed first: the BashWrapperBuilder half of the fix never fires for non-Fusion grid arrays (SLURM/SGE/PBS/PBS Pro/LSF/TCS combined with Docker/Singularity/Podman/etc.), so the array-index variable is silently dropped from the task container on those platforms.

Why

arrayIndexName is populated on TaskBean only when task instanceof TaskArrayRun (TaskBean.groovy:180-183). But:

  • the TaskArrayRun has isContainerEnabled() == false, so its own container builder is never invoked;
  • for a non-Fusion grid array the actual docker run/singularity exec is emitted into each child's .command.run, built from a plain child TaskRun via TaskArrayCollector.createTaskArray()handler.prepareLauncher()GridTaskHandler.createTaskWrapper(childTask)new TaskBean(childTask), where arrayIndexName is null.

withArrayChild(true) only flags the handler (TaskHandler.isArrayChild), never the bean, so if( arrayIndexName ) in createContainerBuilder() is skipped and the container launches without -e SLURM_ARRAY_TASK_ID.

The Fusion path works because Fusion is container-native and wraps the container at array-submit time via FusionHelper.runWithContainer() using the TaskArrayRun bean (which does carry arrayIndexName). AWS/Google Batch are unaffected — they inject the array index natively and never read getEnvWhitelist().

Note that the added BashWrapperBuilderTest passes because it stubs getArrayIndexName() on the builder, which bypasses exactly this broken wiring. I'll push a fix + a handler-level test that exercises the real child path.

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 <jorge.ejarque@seqera.io>

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: jorgee <jorge.ejarque@seqera.io>
@jorgee

jorgee commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Pushed 21aef4f to close the gap. Three parts:

1. Fix — propagate the index name to the child launcher (GridTaskHandler.createTaskWrapper())

The handler is the one place that knows both that it's building an array child (isArrayChild) and which TaskArrayExecutor is in play, so it now injects the executor's array index name onto the (non-Fusion) child builder:

protected BashWrapperBuilder createTaskWrapper(TaskRun 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
}

This reuses the whitelist branch already added in BashWrapperBuilder.createContainerBuilder() — no new mechanism. Scoped to non-Fusion because the Fusion path already handles it at array-submit time via FusionHelper.runWithContainer() with the TaskArrayRun bean. All grid TaskArrayExecutors (SLURM/SGE/PBS/PBS Pro/LSF/TCS) route through this method; AWS/Google Batch inject the index natively.

2. Guard — the ### array: metadata block (BashWrapperBuilder.getTaskMetadata())

Now that array children carry arrayIndexName, the metadata block is gated on arrayWorkDirs too:

if( bean.arrayIndexName && bean.arrayWorkDirs ) {

Only the dispatcher task (TaskArrayRun) carries arrayWorkDirs; without this guard a child would render a bogus ### array: block and NPE on for( Path it : bean.arrayWorkDirs ).

3. Test — exercise the real child path (GridTaskHandlerTest)

Added should propagate the array index variable to a containerised array child launcher: it builds the child launcher from a plain TaskRun (asserting childBuilder.arrayIndexName == null as the precondition — the actual broken state), runs it through the real createTaskWrapper(), and asserts the handler injects SLURM_ARRAY_TASK_ID. A companion test confirms a non-array task is untouched and never queries getArrayIndexName(). I verified the positive test fails against the pre-fix createTaskWrapper().

I left the existing BashWrapperBuilderTest case as-is — it's a valid unit test of the whitelist/addEnv consumption side; the new handler test covers the propagation side that was missing.

@jorgee jorgee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found an issue in the non-fusion path and pushed a fix and extra tests to cover this case. Ready to merge from my side.

@bentsherman

Copy link
Copy Markdown
Member Author

Thanks for catching this edge case. However I need to review this PR as a whole again. I'm concerned about polluting the code too much with all of this array logic. Need to see if we can do it in a more isolated manner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

java.util.ConcurrentModificationException when using array jobs in Slurm

2 participants