feat(storage): add BinaryAssetStorageAPI and local cache eviction for S3 asset storage

[swicken]

S3 asset storage, part 2 of 7. Stacked PRs, review bottom up. Each one builds on the one below.
1a storage layer #37770 · 1b binary asset API #37771 · 2 content #37772 · 3 recovery #37773 · 4 publishing #37774 · 5 temporary uploads and WebDAV #37775 · 6 rendering #37776
Everything is behind FEATURE_FLAG_S3_ASSET_STORAGE, off by default. With the flag off, behavior matches main.

Refs #37868

Proposed Changes

• BinaryAssetStorageAPI (APILocator.getBinaryAssetStorageAPI()) manages binary assets (binary-assets group) and completed renditions (generated-assets group) through the storage chain. With the flag on, keys in these two groups keep their mixed-case names; other groups are still lowercased (FileSystemStoragePersistenceAPIImpl.normalizePath). Inode, field and file names are validated, and directory deletes must resolve under the asset root.
• Content-addressed S3 bytes for those groups. The owned key holds a small reference to a SHA-256 blob, so identical bytes share one copy. Uploads hash a private snapshot, cold downloads verify SHA-256, and backfill converts raw objects only if their ETag still matches. Deleting an owner removes its reference, never the shared blob.
• Cache leases let callers hold a local file safely through a read. FileStorageAPIImpl uses one while metadata generation needs an evicted original's bytes.
• BinaryCacheEvictionJob trims each node's local cache, scheduled on every node from the node-local scheduled pool only when BINARY_CACHE_EVICTION_CRON is set. A file is evicted only when S3 holds a verified copy at the exact key (the reference's SHA-256 must match the local file, then the blob's bytes are compared). The verification runs without any lock; the write lock is taken only to re-check the file's attributes and delete it, so a slow S3 never blocks reads.

Behavior with the flag off

Unchanged from main. The API uses the existing filesystem/NFS paths and makes no remote calls, the eviction job is not scheduled, and the scheduler logs nothing new. The name validation also applies with the flag off, where it only rejects invalid input; nothing in dotCMS calls these methods with the flag off.

Review fixes

The last commit on this branch (fix(storage): verify eviction without the cache lock and harden the binary asset API) addresses a full review of this PR:

• Eviction no longer holds the process-wide write lock through the S3 download and byte comparison, which stalled every binary read on the node.
• Eviction runs on every node. As a clustered Quartz job it fired on only one node per run, while the cache it trims is per node. A copy stored by an earlier build is removed at startup.
• A row whose contentlet_as_json is null or blank falls back to the older lookup instead of reporting its binaries as missing.
• The older-layout methods validate their inputs, so deleteAllBinaries("..") can no longer delete the parent of the asset root.
• The read lock is taken only with the flag on, the generated-group check has a fast path, and metadata generation for a missing file gives the flag-off results again.
• The doc describes the durable-copy proof actually used and no longer claims that NFS-backed directories never evict; it says not to set the eviction cron on nodes that share an asset directory.

Deliberately not in this PR

No content code calls the API yet; check-in, reads and cleanup arrive in 2. The storageKey fields on the binary field type also move to 2, because only 2 reads them.

Checklist

• Tests: the 115 unit tests in the doc's run command pass after the review fixes (1 skip: an STS case that needs extra setup), including the MinIO cases against a disposable bucket. No integration tests yet; every CMS integration case needs check-in from 2.
• Translations: not applicable.
• Security Implications Contemplated: eviction rejects symlinks, paths outside the configured asset and dotGenerated roots, and files without a verified remote copy. Inode, field and file names are validated before any path is built.

[claude]

Claude finished @swicken's task in 2m 9s —— View job

---

Code Review

Reviewed the diff against origin/s3-stack/1a-storage-layer. This is a flag-gated addition (FEATURE_FLAG_S3_ASSET_STORAGE, default off); with the flag off the paths delegate to the existing filesystem behavior. Validation and path-escape guards throughout BinaryAssetReference / BinaryAssetStorageAPIImpl are thorough, and the lock-free eviction verification (evictLocalFile) correctly takes the write lock only for the final re-check-and-delete. No P0/P1 data-loss or security defects found.

Existing (prior dotbot findings — still present)

• 🟡 Medium: dotCMS/src/main/java/com/dotcms/storage/binary/BinaryAssetStorageAPIImpl.java:543 — copyBinary non-filesystem branch dereferences a null source. When the source field has no binary, both getBinaryFile(sourceInode, …) and pullFile(BINARY_ASSETS_GROUP, sourcePath) return null; the following pushFile(…, destPath, sourceFile, …) then opens sourceFile.toPath() inside S3ContentAddressedStorage.store, producing an opaque NPE wrapped as DotDataException. The filesystem branch (line 533, FileUtil.copyFile) already fails with a clear message on the same missing-source input, so the two modes are inconsistent. Add a guard before pushFile:

  if (sourceFile == null || !sourceFile.exists()) {
      throw new DotDataException(String.format(
          "Source binary does not exist for inode '%s', field '%s', file '%s'",
          sourceInode, fieldVarName, fileName));
  }

  Latent today (the PR notes no content code calls the API yet), so non-blocking. Fix this →

• 🟡 Medium: dotCMS/src/main/java/com/dotmarketing/init/DotInitScheduler.java:619 — the eviction chain self-reschedules onto the shared scheduled pool with no retained ScheduledFuture and no started-guard. scheduleBinaryCacheEviction is re-invoked in its own finally, so if DotInitScheduler.start() is ever re-entered, each entry spawns an independent, uncancellable eviction chain for the JVM lifetime. Low impact as long as production InitServlet calls start() once per JVM; a static started-guard (or a retained+cancelled ScheduledFuture) makes it robust if that ever changes. Fix this →

Notes (not blocking)

• The rollback comment (H-5) is a release/deploy concern, not a code defect: once the flag is enabled, bytes written under the content-addressed reference scheme are not readable by an N-1 build that lacks sharedBytes(). The read side already falls back to raw-object reads, but there is no equivalent protection for a new-scheme object read by N-1. Keep the flag off in any environment inside a rollback window and call it out as rollback-unsafe-if-enabled in release notes. Already acknowledged in the PR thread.

No new issues beyond the two carried-forward findings.

· s3-stack/1b-binary-api

[github-actions]

�f7e1 [P2] BinaryAssetStorageAPIImpl.java:543 guard missing source in copyBinary

Current code:

final File sourceFile = resolveContentReferences && com.dotcms.storage.AssetStorageFeature.isEnabled()
        ? getBinaryFile(sourceInode, fieldVarName, fileName)
        : storagePersistenceAPI.pullFile(BINARY_ASSETS_GROUP, sourcePath);
final String destPath = buildFilePath(destInode, fieldVarName, fileName);
storagePersistenceAPI.pushFile(BINARY_ASSETS_GROUP, destPath, sourceFile, Map.<String, Serializable>of());

Problem: Missing source returns null, then push dereferences it opaquely.

Fix:

final File sourceFile = resolveContentReferences && com.dotcms.storage.AssetStorageFeature.isEnabled()
        ? getBinaryFile(sourceInode, fieldVarName, fileName)
        : storagePersistenceAPI.pullFile(BINARY_ASSETS_GROUP, sourcePath);
if (sourceFile == null) {
    throw new DotDataException(String.format("Binary not found for inode '%s', field '%s', file '%s'", sourceInode, fieldVarName, fileName));
}
final String destPath = buildFilePath(destInode, fieldVarName, fileName);
storagePersistenceAPI.pushFile(BINARY_ASSETS_GROUP, destPath, sourceFile, Map.<String, Serializable>of());

[github-actions]

🟡 [P2] BinaryAssetStorageAPIImpl.java:543 guard missing source in copyBinary

Current code:

                final File sourceFile = resolveContentReferences && com.dotcms.storage.AssetStorageFeature.isEnabled()
                        ? getBinaryFile(sourceInode, fieldVarName, fileName)
                        : storagePersistenceAPI.pullFile(BINARY_ASSETS_GROUP, sourcePath);

                final String destPath = buildFilePath(destInode, fieldVarName, fileName);
                storagePersistenceAPI.pushFile(BINARY_ASSETS_GROUP, destPath, sourceFile,
                        Map.<String, Serializable>of());

Problem: Both getBinaryFile and pullFile return null when the source field has no binary; pushFile then dereferences it (S3ContentAddressedStorage.store opens source.toPath()), producing an opaque NPE wrapped as DotDataException.

Fix:

                final String destPath = buildFilePath(destInode, fieldVarName, fileName);
                if (sourceFile == null || !sourceFile.exists()) {
                    throw new DotDataException(String.format(
                            "Source binary does not exist for inode '%s', field '%s', file '%s'",
                            sourceInode, fieldVarName, fileName));
                }
                storagePersistenceAPI.pushFile(BINARY_ASSETS_GROUP, destPath, sourceFile,
                        Map.<String, Serializable>of());

The filesystem branch already fails with a clear DotDataException on the same missing-source input; this makes both modes consistent and descriptive.

[github-actions]

⚪ [P3] DotInitScheduler.java:619 eviction chain cannot be cancelled and duplicates if start() re-enters

Current code:

			} finally {
				scheduleBinaryCacheEviction(cron, new Date());
			}
		}, Math.max(0, next.getTime() - System.currentTimeMillis()), TimeUnit.MILLISECONDS);

Problem: scheduleBinaryCacheEviction self-reschedules onto the shared scheduled pool with no retained ScheduledFuture or started-guard. If DotInitScheduler.start() is ever re-entered, each entry spawns an independent, uncancellable eviction chain for the JVM lifetime.

Fix:

	private static final java.util.concurrent.atomic.AtomicReference<java.util.concurrent.ScheduledFuture<?>> BINARY_EVICTION_CHAIN =
			new java.util.concurrent.atomic.AtomicReference<>();

	private static void scheduleBinaryCacheEviction(final CronExpression cron, final Date after) {
		final Date next = cron.getNextValidTimeAfter(after);
		if (next == null) {
			Logger.warn(DotInitScheduler.class, "BinaryCacheEvictionJob cron expression has no future run: " + cron);
			return;
		}
		final java.util.concurrent.ScheduledFuture<?> future = DotConcurrentFactory.getScheduledThreadPoolExecutor().schedule(() -> {
			try {
				new BinaryCacheEvictionJob().execute(null);
			} catch (final Exception e) {
				Logger.warnAndDebug(DotInitScheduler.class, e);
			} finally {
				scheduleBinaryCacheEviction(cron, new Date());
			}
		}, Math.max(0, next.getTime() - System.currentTimeMillis()), TimeUnit.MILLISECONDS);
		BINARY_EVICTION_CHAIN.set(future);
	}

Assumption: production InitServlet calls start() once per JVM, so impact today is low; the guard makes it robust if that guarantee ever changes.

[claude]

test

[claude]

Pull Request Unsafe to Rollback!!!

• Category: H-5 — Binary Storage Provider Change
• Risk Level: 🟠 HIGH
• Why it's unsafe: With FEATURE_FLAG_S3_ASSET_STORAGE on, writes to the binary-assets/generated-assets S3 groups no longer put the real file bytes at the object's normal key. S3ContentAddressedStorage.store() (used via the new sharedBytes() helper) uploads the actual bytes once under a dedup key (asset-blobs/sha256/...) and overwrites the owner key with a small JSON pointer object ({"sha256":...,"length":...}, marked by the dotcms-blob-sha256 S3 user-metadata header). N-1's AmazonS3StoragePersistenceAPIImpl has no sharesBinaryBytes()/sharedBytes() concept — it calls the plain storage.downloadFile(bucketName, transformReadPath(groupName, path), download) path, which fetches that JSON pointer and treats it as the binary itself. Every file download, image render, or metadata read for binaries written under the new scheme returns the literal JSON reference instead of the real content after a rollback. This is confirmed by the PR's own doc update: "the owned key holds a small reference object... With the flag on, keys in both groups keep their mixed-case names" (docs/testing/BINARY_S3_STORAGE.md).
• Code that makes it unsafe:
  • dotCMS/src/main/java/com/dotcms/storage/AmazonS3StoragePersistenceAPIImpl.java — new sharesBinaryBytes(String group) / sharedBytes() helpers, and the pushFile/pullFile/existsInStorage/backfill branches that call sharedBytes().store(...) / sharedBytes().retrieve(...) instead of the legacy direct S3 object read/write when the flag is on.
  • dotCMS/src/main/java/com/dotcms/storage/FileSystemStoragePersistenceAPIImpl.java — normalizePath() now keeps mixed-case keys for binary-assets/generated-assets instead of always lowercasing, a second format divergence N-1 doesn't know about.
  • (Pre-existing, not changed by this PR, but is what the new code calls into) dotCMS/src/main/java/com/dotcms/storage/S3ContentAddressedStorage.java#store/retrieve.
• Alternative (if possible): This already follows H-5's "support reading from both old and new locations" idea on the read side (retrieve() falls back to raw-object reads), which is good — but there's no equivalent protection for a binary written under the new reference scheme being read by N-1 after a rollback. Since the feature is still behind AssetStorageFeature.isEnabled() (default false) and not yet enabled by default, the two-phase migration pattern is already partially in place; just make sure the flag is never turned on in any environment that might need a rollback until N-1 (or an equivalent dual-read capability) is fully retired from the rollback window, and call out the flag explicitly as rollback-unsafe-if-enabled in release notes.

[github-actions]

dotbot code review:

• Reviewer: meta/muse-spark-1.3 (medium)
• Overall: patch is incorrect
• New findings this run: 0
• Prior unresolved dotbot findings still relevant: 2
• Active findings total: 2

No new actionable bugs were found in the current changes, but 2 prior unresolved dotbot findings still apply, so the patch remains incorrect.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

_{reviewed by dotbot · meta/muse-spark-1.3 · medium}

[github-actions]

dotbot code review:

• Reviewer: ~z-ai/glm-latest (medium)
• Overall: patch is incorrect
• New findings this run: 0
• Prior unresolved dotbot findings still relevant: 2
• Active findings total: 2

No new actionable bugs were found in the current changes, but 2 prior unresolved dotbot findings still apply, so the patch remains incorrect.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

_{reviewed by dotbot · ~z-ai/glm-latest · medium}