fix(contentlet): stop timestamp preservation from failing binary copies on shared storage (#37068)

[fabrizzio-dotCMS]

Fixes #37068

Problem

Copying a contentlet with a binary field aborts on NFS/EFS-backed asset volumes (clustered Cloud environments) with:

com.dotmarketing.exception.DotDataException: Error copying binary file: 'example-file_copy.dmg': Cannot set the file time.
Caused by: java.io.IOException: Cannot set the file time.
	at org.apache.commons.io.FileUtils.copyFile(FileUtils.java:815)

It surfaces as a WorkflowActionFailureException, so the Copy workflow action fails — and in a bulk copy run (fireBulkActionTasks) one bad file kills the task.

Root cause

ESContentletAPIImpl.copyContentlet(...) staged each binary through the two-argument commons-io helper, which delegates with preserveFileDate = true:

public static void copyFile(final File srcFile, final File destFile) throws IOException {
    copyFile(srcFile, destFile, StandardCopyOption.REPLACE_EXISTING);  // -> preserveFileDate = true
}

public static void copyFile(final File srcFile, final File destFile,
        final boolean preserveFileDate, final CopyOption... copyOptions) throws IOException {
    ...
    Files.copy(srcFile.toPath(), destFile.toPath(), copyOptions);
    if (preserveFileDate && !setTimes(srcFile, destFile)) {
        throw new IOException("Cannot set the file time.");   // <-- thrown here
    }
}

setTimes(...) tries BasicFileAttributeView.setTimes(...) then falls back to File.setLastModified(long). On NFS/EFS-style mounts both can fail.

The byte copy has already succeeded at that point. Only the timestamp-preservation step fails, and the destination is a brand-new file under a throwaway UUID temp folder (getRealAssetPathTmpBinary() + /<uuid>), so the source's modification time carries no business meaning. A cosmetic, best-effort operation was aborting a user-facing workflow action.

The line dates to 2013 and is unchanged on main — it only surfaces where the timestamp call is not permitted.

Fix

  destFile = new File(temporalFolder + File.separator + fieldValue);
- if (!destFile.exists()) {
-     destFile.createNewFile();
- }
- FileUtils.copyFile(srcFile, destFile);
+ FileUtils.copyFile(srcFile, destFile, false, StandardCopyOption.REPLACE_EXISTING);

Two details worth reviewing:

1. REPLACE_EXISTING must be passed explicitly. The three-arg form is copyFile(src, dest, boolean, CopyOption...) — writing just copyFile(src, dest, false) passes zero copy options, and Files.copy then fails if the destination exists. This is not merely defensive: destFile is temporalFolder + "/" + srcFile.getName() and temporalFolder is shared across all fields of the contentlet, so a content type with two binary fields holding files of the same name writes twice to the same path. That case was previously covered by createNewFile() plus the implicit REPLACE_EXISTING of the two-arg overload.
2. Real I/O failures still propagate. Nothing is swallowed — a genuine failure in Files.copy (source unreadable, disk full, destination not writable) still throws and is wrapped as DotDataException by the existing catch block.

The now-redundant createNewFile() is removed: commons-io creates parent directories and the destination file itself.

Second call site

FileSystemStoragePersistenceAPIImpl.pushFile(...) had the same defect writing into the asset bucket on the same shared volume, and is fixed identically. REPLACE_EXISTING there preserves the previous overwrite semantics exactly.

Audited but not changed

These share the preserveFileDate = true default but are not on the shared asset path and have no reports — flagged for a follow-up rather than widened here:

Location	Call
PublisherAPIImpl:379	copyFile (bundle build)
FsFileResource:117	copyFile (WebDAV)
FsDirectoryResource:122	copyDirectory (WebDAV)
OSGIUtil:1190	copyDirectory (bundle dir bootstrap)

Tests

Two integration tests added to ESContentletAPIImplTest (already registered in MainSuite3a):

• copyContentletWithBinaryFieldKeepsFileContent — the copy carries its own binary, byte-identical to the source, not a pointer at the source file.
• copyContentletWithTwoBinaryFieldsSharingFileName — two binary fields whose files share a name; guards the REPLACE_EXISTING option described above. Without it this fails with FileAlreadyExistsException.

Known coverage gap — the setTimes failure itself is not simulated. There is no portable way to force it in a test: as root in CI the modification time can always be set, and a fake wedge would not reproduce the real condition. The tests cover the copy contract and the REPLACE_EXISTING regression; validating the original NFS/EFS scenario requires a manual check on a clustered environment (see QA note below).

Verification

• dotcms-core compiles and installs clean.
• dotcms-integration test-compiles clean (BUILD SUCCESS).
• openapi.yaml unchanged — no annotation changes in this PR.
• The integration tests were not executed locally (they need the Postgres + Elasticsearch stack); they are left to CI.

QA

On a clustered Cloud environment with shared asset storage:

1. Upload a File Asset (or any content type with a binary field).
2. Run the Copy workflow action on it — single, and via a bulk workflow action over a selection.
3. The copy completes; the new content has its binary attached and downloadable, identical to the original.
4. dotcms.log shows no Cannot set the file time. and no WorkflowActionFailureException.

🤖 Generated with Claude Code

[claude]

Claude finished @fabrizzio-dotCMS's task in 1m 14s —— View job

---

PR Review

• Read the diff
• Examined changed source in context
• Verified copyFile overload semantics & test helpers
• Posted review

I reviewed the diff against origin/main. This is a tight, well-scoped fix. I verified the key claims:

• commons-io version supports the overload — bom/application/pom.xml:702 pins commons-io 2.14.0; the copyFile(File, File, boolean, CopyOption...) overload exists (since 2.8.0). ✅
• Dropping createNewFile() is safe — all copyFile overloads delegate to the same core that calls createParentDirectories(destFile) before Files.copy, so parent dirs and the destination file are created regardless. ✅
• REPLACE_EXISTING correctly restores prior overwrite semantics — the old two-arg form implied REPLACE_EXISTING + preserveFileDate=true; the new call keeps overwrite behavior and only drops the date preservation. Passing false without an explicit copy option would have regressed to FileAlreadyExistsException, which the second test guards against. ✅
• Real I/O errors still propagate — ESContentletAPIImpl wraps in DotDataException (line 9594-9598), FileSystemStoragePersistenceAPIImpl logs + rethrows (line 188-191). Nothing is swallowed. ✅
• Test helpers/imports all resolve — createFile (line 424), FileTestUtil (import line 48), com.liferay.util.FileUtil (line 100), java.nio.file.Files (line 98) are all present. ✅

New Issues

No issues found.

The root-cause analysis is accurate: the byte copy at ESContentletAPIImpl.java:9590 and FileSystemStoragePersistenceAPIImpl.java:187 succeeds before the timestamp step, and the destination timestamps carry no business meaning (throwaway UUID temp folder / asset bucket where mod_date lives in the DB, not the filesystem). Best-effort date preservation should not abort a user-facing workflow action.

A couple of non-blocking observations (not defects, no action required):

• 🟡 The known coverage gap is honestly documented — the setTimes failure itself isn't simulated because it can't be forced portably. The two added tests cover the copy contract and the REPLACE_EXISTING regression, which are the parts reproducible in CI; the NFS/EFS path is left to the manual QA steps. That's a reasonable tradeoff.
• 🟡 The four audited-but-unchanged call sites (PublisherAPIImpl, FsFileResource, FsDirectoryResource, OSGIUtil) still carry the preserveFileDate=true default. Scoping them out of this PR is the right call since they're off the shared asset path and have no reports — worth a follow-up ticket so the audit isn't lost.

Nice work — the fix is correct, minimal, and the reasoning in the comments will help the next reader understand why false and the explicit REPLACE_EXISTING are both required.
· issue-37068-copy-binary-file-time