GH-11277: Fix local directory creation race in remote file GET

[dlwldn30]

Fixes: #11277

The defect

AbstractRemoteFileOutboundGateway.generateLocalDirectory() creates the local directory with a non-atomic exists()/mkdirs() sequence, per message, on the calling thread:

if (!localDir.exists()) {
	Assert.isTrue(localDir.mkdirs(), () -> "Failed to make local directory: " + localDir);
}

The eager creation in afterPropertiesSet() does not cover this, because it is restricted to a ValueExpression:

if (this.localDirectoryExpression instanceof ValueExpression) {
	setupLocalDirectory();
}

So with a local-directory-expression the directory is never pre-created and creation relies entirely on the per-message path. Concurrent messages resolving to the same, not-yet-existing directory all pass !localDir.exists(), only one mkdirs() wins, and the losing threads get false and fail:

java.lang.IllegalArgumentException: Failed to make local directory: /private/tmp/junit-.../local-0
	at org.springframework.util.Assert.isTrue(Assert.java:136)
	at org.springframework.integration.file.remote.gateway.AbstractRemoteFileOutboundGateway.generateLocalDirectory(AbstractRemoteFileOutboundGateway.java:1431)
	at org.springframework.integration.file.remote.gateway.AbstractRemoteFileOutboundGateway.get(AbstractRemoteFileOutboundGateway.java:1175)

The directory exists at that point and the transfer could have proceeded.

The code is identical in v6.0.0, v6.3.0, v7.0.0 and main.

The change

Use Files.createDirectories(), which is idempotent and does not report an already-existing directory as a failure, and propagate a genuine creation failure as the cause:

if (!localDir.exists()) {
	try {
		Files.createDirectories(localDir.toPath());
	}
	catch (IOException ex) {
		throw new IllegalArgumentException("Failed to make local directory: " + localDir, ex);
	}
}

This mirrors FileWritingMessageHandler, which has used the same call for the same class of problem since #11254.

The two adjusted tests

Attaching the IOException as cause moves the root cause of the reported IllegalArgumentException from itself to a FileSystemException:

MessageHandlingException
  └ MessagingException: Failed to execute on session
      └ IllegalArgumentException: Failed to make local directory     <- was the root cause
          └ FileSystemException: Read-only file system               <- is the root cause now

FtpServerOutboundTests.testInt2866InvalidLocalDirectoryExpression and its SftpServerOutboundTests counterpart pinned that position with hasRootCauseInstanceOf(IllegalArgumentException.class). That held only because Assert.isTrue() left the chain empty, so the exception happened to be its own root - it was not an intentional assertion about the root.

They now assert the same type and message where they occur in the chain rather than at its root:

.hasStackTraceContaining("java.lang.IllegalArgumentException: Failed to make local directory");

hasCauseInstanceOf() would not work either - the IllegalArgumentException sits two levels down, under the MessagingException from the session template.

These assertions pass against the unfixed gateway as well, so they assert the contract rather than encoding this change. Verified by restoring only the production file to the base commit and re-running both classes - green.

Tests

RemoteFileOutboundGatewayTests.testGetConcurrentCreateSameLocalDirectory - 8 threads, 50 rounds, a fresh local directory per round, one distinct remote file per thread so only the destination directory is shared. It takes an isolated @TempDir parameter rather than the shared static one, so it cannot perturb testMputRecursive.

The previous revision was verified only against :spring-integration-file:check, which is how the -ftp and -sftp failures were missed. All three modules that build on this class are now in scope:

module	result
:spring-integration-file:check	332 tests, 0 failures, 8 skipped
:spring-integration-ftp:test	92 tests, 0 failures, 5 skipped
:spring-integration-sftp:test	92 tests, 0 failures, 1 skipped

checkstyleMain and checkstyleTest pass for all three.

Revert check: restoring only the production file with git checkout <base> -- <file> (not git stash, which would have been a no-op after the commit) makes exactly the new test fail - 36 tests completed, 1 failed.

Scope

setupLocalDirectory() has the same shape (exists() then mkdirs(), at line 612) but runs once from afterPropertiesSet() on a single thread, so it cannot race. I left it alone to keep the diff minimal - happy to align it for consistency if you prefer.

AbstractInboundFileSynchronizingMessageSource and FileReadingMessageSource carry the same pattern. The former I have not traced to a concurrent caller; the latter is guarded by running.getAndSet(true) in start(). Both look safe today, but they are the same class of code and might be worth a sweep separately.

No open PR touches AbstractRemoteFileOutboundGateway.java.

Contributed on behalf of Goatshave.

[cppwfs]

Thank you for your contribution!

When building the full project we see that there are test failures in FtpServerOutboundTests and SftpServerOutboundTests.

[dlwldn30]

Reproduced both failures locally - FtpServerOutboundTests.testInt2866InvalidLocalDirectoryExpression and its SftpServerOutboundTests counterpart:

Expecting a throwable with root cause being an instance of:
  java.lang.IllegalArgumentException
but was an instance of:
  java.nio.file.FileSystemException

Both assert the root cause. Attaching the IOException as the cause of the IllegalArgumentException moved the root cause to FileSystemException. My description claimed nothing observable changes - the type and the message were kept, but the added cause is observable, and these two tests observe exactly that. I had only run :spring-integration-file:check, which is why I missed it.

Fixed by leaving the exception untouched and making the mkdirs() result race-tolerant instead:

if (!localDir.exists()) {
	Assert.isTrue(localDir.mkdirs() || localDir.exists(),
			() -> "Failed to make local directory: " + localDir);
}

This is the idiom already used by PropertiesPersistingMetadataStore, and it closes the race the same way - mkdirs() returning false is only a failure when the directory is still absent.

Verification:

• :spring-integration-file:check - 332 tests, 0 failures, 8 skipped; checkstyleMain and checkstyleTest pass
• FtpServerOutboundTests - 24 tests, 0 failures
• SftpServerOutboundTests - 20 tests, 0 failures
• Revert check: restoring only the production file with git checkout HEAD~1 -- <file> makes testGetConcurrentCreateSameLocalDirectory fail again (36 tests completed, 1 failed)

One point for you to weigh rather than me: Files.createDirectories() is what FileWritingMessageHandler has used since #11254, so dropping it here costs that consistency and the underlying IOException as a cause. Keeping it would mean relaxing the root-cause assertion in those two tests. I went with the form that leaves the existing tests untouched, since changing them to accommodate my own change looked like the wrong trade - happy to switch if you prefer the Files.createDirectories() version.

The PR description is updated accordingly.

[artembilan]

I think moving to the Files.createDirectories() is the right direction for consistency and less stacktrace.
Consider to adjust those tests instead.
Even if this is a bit of a breaking change due to different exception, it is likely better than more stacktrace or even race condition.

[dlwldn30]

Thanks - switched to Files.createDirectories() and adjusted the two tests.

One thing worth flagging, since it changed what the adjustment had to look like. The IllegalArgumentException is not the direct cause either, so swapping hasRootCauseInstanceOf for hasCauseInstanceOf does not work - it sits two levels down:

MessageHandlingException
  └ MessagingException: Failed to execute on session
      └ IllegalArgumentException: Failed to make local directory     <- was the root cause
          └ FileSystemException: Read-only file system               <- is the root cause now

The old assertion held only because Assert.isTrue() left the chain empty, so the exception happened to be its own root. Rather than re-pin it to FileSystemException - which would pin a position again, and couple the test to whichever FileSystemException subtype the platform throws - both tests now assert the type and message wherever they occur in the chain:

.hasStackTraceContaining("java.lang.IllegalArgumentException: Failed to make local directory");

These pass against the unfixed gateway too, so they are not written around this change. I checked by restoring only the production file to the base commit and re-running both classes - green.

Verification now covers every module that builds on this class, which is what I should have done the first time:

module	result
:spring-integration-file:check	332 tests, 0 failures, 8 skipped
:spring-integration-ftp:test	92 tests, 0 failures, 5 skipped
:spring-integration-sftp:test	92 tests, 0 failures, 1 skipped

checkstyleMain and checkstyleTest pass for all three. Revert check still valid - restoring only the production file makes exactly the new concurrency test fail.

The PR description is updated accordingly.

[cppwfs]

Thank you again for the contribution!

[dlwldn30]

@cppwfs friendly ping on this one.

Both tests now assert hasRootCauseInstanceOf(FileSystemException.class) alongside the message check, as requested.

The build is still waiting on workflow approval, so for reference, locally:

• :spring-integration-file:check - 332 tests, 0 failures, 8 skipped
• :spring-integration-ftp:test - 92 tests, 0 failures, 5 skipped
• :spring-integration-sftp:test - 92 tests, 0 failures, 1 skipped

checkstyleMain and checkstyleTest pass for all three.

Happy to adjust anything else.

[artembilan]

Use mock() without arguments.

[artembilan]

We don't work with SonarQube any more.
Therefore, we don't need all of these extra noisy comment.

[cppwfs]

Again thank you for the contribution. Also rebase your PR and that will resolve the build failure.

[artembilan]

Just one question.
Thank you!

[artembilan]

How slow is this test?
I mean you made the point with this verification that atomic Java native Files.createDirectories() feature works well.
But is it really our project responsibility to test that method behavior?
I feel like we don't need it at all since it is out of scope.