Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 63 additions & 3 deletions docs/testing/BINARY_S3_STORAGE.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ The storage chain (`ChainableStoragePersistenceAPI`) and its providers change as
completed renditions through the storage chain. Binary assets use the `binary-assets` group under
the asset root, laid out as `{inode[0]}/{inode[1]}/{inode}/{field}/{fileName}`. Renditions use the
`generated-assets` group under the `dotGenerated` root. With the flag on, keys in these groups keep
their mixed-case names, as do publishing bundles (below); other groups are still lowercased.
their mixed-case names, as do publishing bundles and temporary uploads (below); other groups are
still lowercased.

With the flag on, S3 keeps one immutable copy of each set of bytes for these two groups, at
`asset-blobs/sha256/<chars-1-2>/<chars-3-4>/<chars-5-6>/<chars-7-8>/<sha256>`
Expand Down Expand Up @@ -327,6 +328,65 @@ that is already gone succeeds.

With the flag off, bundles are written, read and deleted in the bundle directory as before.

## Temporary uploads

With the flag on, completed temporary uploads (`TempFileAPI`, including the image editor's output)
are stored in the `temporary-assets` group, with an immutable receipt that records who may use the
upload and when it expires. Access and expiry are checked before any bytes are downloaded, so
another node can serve the upload after restoring it into its own local root. Mixed-case and
nested names are kept. A payload is published only after the upload stream has closed; an
interrupted upload keeps any existing complete file and never exposes a partial one. Managed
uploads without a receipt cannot fall back to legacy local access.

Custom metadata for a temporary upload, such as a focal point set before check-in, is stored with
the upload in S3 and read directly from S3, so an edit made on another node is visible. The image
filter's focal point for existing content uses an id made of `temp_` and the content inode. That id
never gets an upload receipt, so its metadata stays in the local temporary directory, as with the
flag off, where `BinaryCleanupJob` removes it after `CLEANUP_TMP_FILES_OLDER_THAN_HOURS`. It is
therefore visible only on the node that wrote it, which the sticky sessions a cluster already
needs make sufficient. Metadata edits are a read, merge and unconditional write, so two nodes
setting different attributes on the same upload at the same moment can lose one of the two
writes. This is accepted because temporary metadata lives only until check-in.

The scheduled `BinaryCleanupJob` removes the S3 objects of an expired upload (its receipt, payload,
metadata and renditions) once `TEMP_RESOURCE_MAX_AGE_SECONDS` has passed. Local copies are left to
the existing `CLEANUP_TMP_FILES_OLDER_THAN_HOURS` age rule, as with the flag off, so a check-in that
resolved the file just before it expired keeps its source; the local upload marker stops an expired
copy from being served. Each upload is cleaned on its own: a failure is logged, that upload keeps
its receipt and payload, the remaining uploads are still cleaned, and the job reports one combined
failure and retries on the next run.

## WebDAV temporary files

With the flag on, WebDAV temporary files (the scratch files some clients write before the final
upload) keep completed payloads and discoverable path records in the `webdav-temporary` group, so
another node can list and read them. A writer reserves a unique payload, uploads it, and then
publishes the path record with an S3 conditional write, so a stale writer cannot publish after a
deletion. Reads materialize immutable local cache files, so an earlier request keeps its bytes
through a later replacement. Mixed-case temporary names are kept, while normal CMS URLs keep their
case-insensitive handling.

Cleanup uses S3 server timestamps, protects completed and pending payload references, and
conditionally replaces expired path records with tombstones. Tombstones are never deleted, because
some S3-compatible stores (MinIO among them) ignore the condition on a conditional delete, and an
unconditional delete could remove a newer upload that reused the path. The group therefore keeps
one small tombstone record per deleted WebDAV temporary path, and each cleanup run reads them
again, until safe reclamation is added. Normal WebDAV uploads still go through content check-in,
and their reads, copies and overwrites open the file under the cache lease through
`Contentlet.getBinaryStream`. With the flag off, WebDAV and temporary uploads use the local
filesystem as before.

A WebDAV COPY of a CMS file creates an unpublished working version on every mount, with either flag
setting, so it needs only edit permission. A folder listing (PROPFIND) reads only the records that
decide each direct temporary child: the child's own record, and only for a folder without a live
record of its own, its descendants until one live descendant is found. Each file resource is built
from the record the listing read, so a temporary file that a client deletes during the listing is
skipped instead of failing it. If S3 cannot be read, the failure is logged and only the temporary
children are left out, so the CMS files and folders still list. The remaining cost is one S3 read
per direct child record on every listing, and that includes the tombstone of every deleted direct
child, so it grows with the number of temporary names ever written directly in that folder until
tombstones can be reclaimed.

## Local cache eviction

With the flag on, the local asset directory is a cache that `BinaryCacheEvictionJob` can trim.
Expand Down Expand Up @@ -425,7 +485,7 @@ docker run -d --rm --name binary-cleanup-postgres-test \
-e POSTGRES_DB=binary_storage_test postgres:16-alpine

./mvnw test -pl :dotcms-core -Dmaven.build.cache.enabled=false \
-Dtest=AssetStorageFeatureTest,AssetStorageFeatureLatchTest,S3StorageConfigurationTest,NoWebIdentityCredentialsProviderChainTest,BinaryS3StorageTest,BinaryAssetReferenceTest,BinaryCacheEvictionJobTest,BinaryFileSystemStorageTest,BinaryAssetStorageAPIImplTest,MetadataLocalCacheTest,BinaryAssetCleanupTransactionTest,BinaryAssetCleanupProcessorTest,ContentletBackupStorageGateTest,BinaryFieldCleanupProcessorTest,AssetJobEventSerializationTest,BinaryAssetBackfillCheckpointTest,ImportStarterWorkflowCleanupTest,BinaryAssetBackfillProcessorTest,BinaryAssetBackfillTest,ExportStarterFailureTest,BundleArchiveStorageTest,FileAssetBundlerTest \
-Dtest=AssetStorageFeatureTest,AssetStorageFeatureLatchTest,S3StorageConfigurationTest,NoWebIdentityCredentialsProviderChainTest,BinaryS3StorageTest,BinaryAssetReferenceTest,BinaryCacheEvictionJobTest,BinaryFileSystemStorageTest,BinaryAssetStorageAPIImplTest,MetadataLocalCacheTest,BinaryAssetCleanupTransactionTest,BinaryAssetCleanupProcessorTest,ContentletBackupStorageGateTest,BinaryFieldCleanupProcessorTest,AssetJobEventSerializationTest,BinaryAssetBackfillCheckpointTest,ImportStarterWorkflowCleanupTest,BinaryAssetBackfillProcessorTest,BinaryAssetBackfillTest,ExportStarterFailureTest,BundleArchiveStorageTest,FileAssetBundlerTest,TemporaryAssetStorageTest,WebdavAssetStorageTest,TemporaryMetadataStorageTest,WebdavTemporaryStorageTest \
-Ds3.test.endpoint=http://127.0.0.1:19002 \
-Ds3.test.jdbc=jdbc:postgresql://127.0.0.1:19003/binary_storage_test

Expand All @@ -438,7 +498,7 @@ stack:
```sh
./mvnw install -pl :dotcms-core --am -DskipTests -Ddocker.skip
./mvnw verify -pl :dotcms-integration -Dmaven.build.cache.enabled=false -Dcoreit.test.skip=false \
-Dit.test=BinaryAssetStorageIntegrationTest,ContentletBackupStorageTest,SharedAssetStorageIntegrationTest,BinaryAssetStarterRestoreTest,PublishingArchiveStorageTest
-Dit.test=BinaryAssetStorageIntegrationTest,ContentletBackupStorageTest,SharedAssetStorageIntegrationTest,BinaryAssetStarterRestoreTest,PublishingArchiveStorageTest,DotWebdavHelperTest
```

These default to flag-off mode, where the S3 cases are skipped. To run the S3 cases, create a
Expand Down
87 changes: 75 additions & 12 deletions dotCMS/src/main/java/com/dotcms/rest/api/v1/temp/TempFileAPI.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import com.dotcms.http.CircuitBreakerUrl;
import com.dotcms.http.CircuitBreakerUrl.Method;
import com.dotcms.rest.exception.BadRequestException;
import com.dotcms.storage.AssetStorageFeature;
import com.dotcms.storage.TemporaryAssetStorage;
import com.dotcms.util.CloseUtils;
import com.dotcms.util.ConversionUtils;
import com.dotcms.util.SecurityUtils;
Expand Down Expand Up @@ -112,7 +114,14 @@ public DotTempFile createEmptyTempFile(final String incomingFileName,final HttpS
final String tempFileId = TEMP_RESOURCE_PREFIX + UUIDGenerator.shorty();

final String tempFileUri = File.separator + tempFileId + File.separator + incomingFileName;
final File tempFile = new File(APILocator.getFileAssetAPI().getRealAssetPathTmpBinary() + tempFileUri);
final File tempFile;
try {
tempFile = AssetStorageFeature.isEnabled()
? TemporaryAssetStorage.getInstance().file(tempFileId, incomingFileName)
: new File(APILocator.getFileAssetAPI().getRealAssetPathTmpBinary() + tempFileUri);
} catch (IOException e) {
throw new DotRuntimeException("Invalid temporary upload path", e);
}
final File tempFolder = tempFile.getParentFile();

if (!tempFolder.mkdirs()) {
Expand All @@ -126,6 +135,14 @@ public DotTempFile createEmptyTempFile(final String incomingFileName,final HttpS
throw new DotRuntimeException("Invalid file upload");
}
createTempPermissionFile(tempFolder, allowList);
if (AssetStorageFeature.isEnabled()) {

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.

mark temp file managed only after completeTempFile succeeds, not during create

Current code:

if (AssetStorageFeature.isEnabled()) {
  try {
    Files.writeString(java.nio.file.Path.of(com.dotmarketing.util.ConfigUtils.getAssetTempPath(),
            tempFileId, TemporaryAssetStorage.MANAGED_MARKER), "");

Problem: The .s3-upload marker is written at creation, before any receipt exists. If the upload never completes (client abort, or store() fails on an S3 outage), the local bytes are fenced off: getTempFile returns empty because isManagedLocally is true but receipt() is absent, and there is no legacy fallback. Until BinaryCleanupJob ages the file out, a valid local copy is unreadable even though the flag-off path would serve it. Consider writing the marker only after store() verifies the S3 copy (store() already writes it), or removing the marker in store()'s failure path.

try {

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.

🟡 [P2] TempFileAPI.java:139 managed marker written before upload completes, fencing servable local bytes

Current code:

if (AssetStorageFeature.isEnabled()) {
  try {
    Files.writeString(java.nio.file.Path.of(com.dotmarketing.util.ConfigUtils.getAssetTempPath(),
            tempFileId, TemporaryAssetStorage.MANAGED_MARKER), "");

Problem: Marker is written at create time, before any receipt exists. If the upload aborts or store() fails on an S3 outage, getTempFile sees isManagedLocally true with no receipt and returns empty with no legacy fallback, fencing a valid local copy until cleanup ages it out.

Fix:

if (AssetStorageFeature.isEnabled()) {
  // marker is written by TemporaryAssetStorage.store() once the S3 copy is verified
}

(store() already writes owner.resolve(MANAGED_MARKER); writing it only there, or deleting it in store()'s failure path, closes the window.)

Files.writeString(java.nio.file.Path.of(com.dotmarketing.util.ConfigUtils.getAssetTempPath(),
tempFileId, TemporaryAssetStorage.MANAGED_MARKER), "");
} catch (IOException e) {
throw new DotRuntimeException("Unable to mark temporary upload", e);
}
}
SecurityLogger.logInfo(this.getClass(),"Temp File Created with id: " + tempFileId + ", uploaded by userId: " + user.getUserId());
return new DotTempFile(tempFileId, tempFile);
}
Expand Down Expand Up @@ -174,20 +191,21 @@ public DotTempFile createTempFile(final String incomingFileName,final HttpServle
final File tempFile = dotTempFile.file;
final long maxLength = maxFileSize(request);

try (final OutputStream out = new BoundedOutputStream(maxLength,Files.newOutputStream(tempFile.toPath()))) {
try {
try (final OutputStream out = new BoundedOutputStream(maxLength,Files.newOutputStream(tempFile.toPath()))) {


int read = 0;
final byte[] bytes = new byte[4096];
while ((read = inputStream.read(bytes)) != -1) {
out.write(bytes, 0, read);
}

if (dotTempFile.metadata == null && dotTempFile.file.exists()) {

return new DotTempFile(dotTempFile.id, dotTempFile.file);
if (!AssetStorageFeature.isEnabled()) {
return dotTempFile.metadata == null && dotTempFile.file.exists()
? new DotTempFile(dotTempFile.id, dotTempFile.file) : dotTempFile;
}
}
return dotTempFile;
return completeTempFile(dotTempFile);
} catch (IOException e) {
final String message = APILocator.getLanguageAPI().getStringKey(WebAPILocator.getLanguageWebAPI().getLanguage(request), "temp.file.max.file.size.error").replace("{0}", UtilMethods.prettyByteify(maxLength));
throw new DotStateException(message, e);
Expand All @@ -198,6 +216,24 @@ public DotTempFile createTempFile(final String incomingFileName,final HttpServle
}
}

/** Finish a closed upload, including files written by the image editor. */
public DotTempFile completeTempFile(final DotTempFile temporary) {
DotTempFile completed = temporary.metadata == null && temporary.file.exists()
? new DotTempFile(temporary.id, temporary.file) : temporary;
if (AssetStorageFeature.isEnabled()) {
// DotTempFile can add a detected extension while building its metadata.
if (!completed.file.exists() && !completed.file.getName().equals(completed.fileName)) {
completed = new DotTempFile(completed.id, new File(completed.file.getParentFile(), completed.fileName));
}
try {
TemporaryAssetStorage.getInstance().store(completed.id, completed.file);
} catch (com.dotmarketing.exception.DotDataException e) {
throw new DotRuntimeException("Unable to complete temporary upload", e);
}
}
return completed;
}

/**
* Takes a URL, downloads it and the returns the resulting file as tempFile with a unique ID and
* file handle that can be used to access the temp file. The request will be used to create a
Expand Down Expand Up @@ -246,11 +282,7 @@ public DotTempFile createTempFileFromUrl(final String incomingFileName,
urlGetter.doOut(out);
}

if (dotTempFile.metadata == null && dotTempFile.file.exists()) {

return new DotTempFile(dotTempFile.id, dotTempFile.file);
}
return dotTempFile;
return completeTempFile(dotTempFile);
}

/**
Expand Down Expand Up @@ -360,6 +392,19 @@ private boolean canUseTempFile(final List<String> incomingAccessingList, final D
* @return
*/
public Optional<DotTempFile> getTempFile(final List<String> accessingList, final String tempFileId) {
if (AssetStorageFeature.isEnabled()) {
if (!TemporaryAssetStorage.validId(tempFileId)) return Optional.empty();
try {
final var store = TemporaryAssetStorage.getInstance();
final var record = store.receipt(tempFileId);
if (record.isPresent()) {
return store.retrieve(record.get(), accessingList).map(file -> new DotTempFile(tempFileId, file));
}
if (store.isManagedLocally(tempFileId)) return Optional.empty();
} catch (com.dotmarketing.exception.DotDataException e) {
throw new DotRuntimeException("Unable to retrieve temporary upload", e);
}
}
Optional<DotTempFile> tempFile = getTempFile(tempFileId);
if (tempFile.isPresent() && canUseTempFile(accessingList, tempFile.get())) {
return tempFile;
Expand Down Expand Up @@ -405,6 +450,17 @@ public Optional<DotTempFile> getTempFile(final HttpServletRequest request, final
* @return
*/
public boolean isTempResource(final String tempFileId) {
if (AssetStorageFeature.isEnabled()) {
if (!TemporaryAssetStorage.validId(tempFileId)) return false;
try {
final var store = TemporaryAssetStorage.getInstance();
final var record = store.receipt(tempFileId);
if (record.isPresent()) return store.exists(record.get());
if (store.isManagedLocally(tempFileId)) return false;
} catch (com.dotmarketing.exception.DotDataException e) {
throw new DotRuntimeException("Unable to check temporary upload", e);
}
}
return getTempFile(tempFileId).isPresent();
}

Expand Down Expand Up @@ -456,6 +512,13 @@ public String getRequestFingerprint(final HttpServletRequest request) {
*/
public Optional<String> getTempResourceId(final File file){
try {
if (AssetStorageFeature.isEnabled()) {
final var root = new File(com.dotmarketing.util.ConfigUtils.getAssetTempPath()).getCanonicalFile().toPath();
final var path = file.getCanonicalFile().toPath();
if (!path.startsWith(root) || root.relativize(path).getNameCount() < 2) return Optional.empty();
final String id = root.relativize(path).getName(0).toString();
return isTempResource(id) ? Optional.of(id) : Optional.empty();
}
final String tempResourceId = file.toPath().getParent().getFileName().toString();
if (isTempResource(tempResourceId)) {
return Optional.of(tempResourceId);
Expand Down
Loading
Loading