Skip to content

Commit ca2b3b5

Browse files
committed
fix(storage): keep renditions, compiled CSS and template loading available when S3 misbehaves
With S3 asset storage on, the JPEG filter now predicts the .jpg file it writes, so warm JPEG renditions are found on local disk instead of making S3 requests on every request and failing during an S3 outage. With the flag off the inherited .png prediction is kept, so flag-off cache paths are unchanged. When a filter returns its input unchanged (for example a maximum width larger than the image), the exporter remembers on that node that the predicted output is never produced, so later requests skip the S3 lookup for it, both per filter and for the chain's final output. Only an output written at its predicted path is uploaded, and the upload now runs after the image permit is released. A failed rendition or compiled CSS upload no longer fails the request. The locally produced output is served, a warning is logged, and the file stays local only, which eviction already never removes. CSSAssetStorageTest now asserts this policy instead of the old fail-closed one. The Sass compiler no longer holds the cache lease while Dart Sass runs; it holds it only while copying sources and while reading a stored output. The rendition lease still spans the whole export and is documented as a known limitation. Focal-point reads treat a permission denial or missing content as no focal point again, as with the flag off, so $content.image.fpx keeps working for anonymous visitors; only a DotDataException from the lookup is rethrown. DotResourceLoader reports a storage failure as a non-cached VelocityException only for the file-backed loaders (VTL, macros, legacy VL and includes). Container, template, page and other database loaders keep the cached ResourceNotFoundException. VTLLoader and IncludeLoader only continue to the restore path for a missing file under the asset root. A missing file elsewhere is a plain not-found again, instead of a "POSSIBLE HACK ATTACK" warning. Adds unit tests for the JPEG prediction, warm JPEG and no-op renditions during an outage, a failed rendition upload, focal-point lookups and the Velocity loader failure handling, and updates BINARY_S3_STORAGE.md.
1 parent 7870f0c commit ca2b3b5

12 files changed

Lines changed: 691 additions & 58 deletions

File tree

‎docs/testing/BINARY_S3_STORAGE.md‎

Lines changed: 47 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -392,24 +392,60 @@ tombstones can be reclaimed.
392392
With the flag on, completed image-filter outputs are uploaded to the `generated-assets` group as
393393
they are produced, at `generated-assets/{first}/{second}/{inode}/dotGenerated_...`, with the same
394394
relative layout under the local `dotGenerated` root. A cold read restores a completed rendition
395-
from S3 without running the filter again. Warm hits do not contact S3, so an S3 outage does not
396-
break a cached response and a warm read cannot republish an invalidated object. Incomplete filter
397-
outputs are never uploaded or evicted. Thumbnail invalidation and source deletion target that
398-
inode's directory. With the flag off, renditions keep the existing two-character layout. Existing
399-
rendition objects are not moved to the new layout.
395+
from S3 without running the filter again. A warm hit is served from local disk without contacting
396+
S3, so an S3 outage does not break a cached response and a warm read cannot republish an
397+
invalidated object. A cold read, which has to look in S3, fails while S3 is unavailable.
398+
Incomplete filter outputs are never uploaded or evicted. Thumbnail invalidation and source
399+
deletion target that inode's directory. With the flag off, renditions keep the existing
400+
two-character layout. Existing rendition objects are not moved to the new layout.
401+
402+
The exporter finds an existing rendition by predicting the path each filter writes, so each
403+
filter's prediction must match its output; the JPEG filter, for example, predicts `.jpg`. Some
404+
requests leave the image unchanged, for instance a maximum width larger than the image. Such a
405+
filter writes nothing, so the first request for it looks for its predicted output in S3 and finds
406+
nothing. The node then remembers that this output is never produced, and later requests on that
407+
node skip the lookup. Only an output written at its predicted path is uploaded.
408+
409+
Upload failures do not fail the request. If S3 rejects a rendition upload, the locally produced
410+
rendition is served and a warning is logged. The rendition then stays local only: later requests
411+
on that node are warm hits and do not retry the upload, eviction keeps the file because it has no
412+
durable copy, and other nodes generate their own. Uploads run after the image-generation permit is
413+
released.
400414

401415
Rendition keys include the original's revision key, so replacing bytes under the same inode and
402416
filename never selects the previous revision's rendition. Crops read the focal point from the
403417
content's metadata snapshot, including focal points saved on a temporary upload, and the Java and
404-
native crop engines use the same pinned coordinates. Image-filter execution holds a cache lease
405-
while it reads the original.
418+
native crop engines use the same pinned coordinates. When a focal point is read through the
419+
content API instead, as `$content.image.fpx` does, content the current user cannot read, or that
420+
does not exist, has no focal point, as with the flag off; only a failed lookup is an error.
406421

407422
Compiled Sass output also uses the `generated-assets` group. Its key covers the source and
408423
dependency bytes, site, live or working mode, compiler options and application build, so a cold
409424
node restores a matching CSS output without running Sass. Source maps keep their existing private,
410-
uncached behavior. Markdown files are read through the protected FileAsset stream, and VTL and
411-
included files are opened through the binary asset API, so a file whose local copy was evicted is
412-
restored from S3 before it is rendered.
425+
uncached behavior. As with renditions, a compiled output whose upload fails is still served,
426+
with a warning, and is kept locally only. Each change to the key, such as an edit to an imported
427+
partial, a working-mode compile of an edit in progress, or an upgrade, stores a new CSS object
428+
without removing the old one. Old objects are removed only when the main Sass file's generated
429+
outputs are invalidated or its inode's binaries are deleted, so for a main file that rarely
430+
changes they accumulate. Superseded compiled CSS objects are not reclaimed yet.
431+
432+
Markdown files are read through the protected FileAsset stream, and VTL and included files are
433+
opened through the binary asset API, so a file whose local copy was evicted is restored from S3
434+
before it is rendered. A missing VTL or included file outside the asset root cannot be restored,
435+
so it is reported as not found, as with the flag off. When restoring a VTL, macro or included file
436+
fails, the render fails with a storage error that is not cached as a missing template, so the next
437+
render tries again. Templates, containers, pages and other database-built sources keep their
438+
existing failure handling.
439+
440+
### Known limitation: long cache leases
441+
442+
Image-filter execution holds a cache lease for the whole export: the S3 lookups, the pixel work
443+
and the uploads of every filter in the chain. Sass compilation holds a lease while it gathers and
444+
copies the sources, which includes database lookups, and while it reads a stored output, but not
445+
while Dart Sass runs. Eviction skips every file while any lease is held, so on a node with steady
446+
image traffic a sweep can be deferred again and again, and the local cache can stay above its
447+
budget. Narrowing the rendition lease to the moments a filter reads a local file needs the filters
448+
to restore their inputs themselves, which is a larger change and is left as a follow-up.
413449

414450
## Local cache eviction
415451

@@ -509,7 +545,7 @@ docker run -d --rm --name binary-cleanup-postgres-test \
509545
-e POSTGRES_DB=binary_storage_test postgres:16-alpine
510546

511547
./mvnw test -pl :dotcms-core -Dmaven.build.cache.enabled=false \
512-
-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,ImageFilterExporterFocalPointTest,ImageFilterExporterTest,ImageFilterExporterSharedStoreTest,ImageFilterExporterEngineSelectionTest \
548+
-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,ImageFilterExporterFocalPointTest,ImageFilterExporterTest,ImageFilterExporterSharedStoreTest,ImageFilterExporterEngineSelectionTest,ImageFilterExporterStorageTest,FocalPointAPIImplTest,StoredVelocityFileLoaderTest \
513549
-Ds3.test.endpoint=http://127.0.0.1:19002 \
514550
-Ds3.test.jdbc=jdbc:postgresql://127.0.0.1:19003/binary_storage_test
515551

‎dotCMS/src/main/java/com/dotcms/csspreproc/DotLibSassCompiler.java‎

Lines changed: 37 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -62,20 +62,23 @@ public DotLibSassCompiler(final Host host, final String uri, final boolean live,
6262
this.sourceMap = sourceMap;
6363
}
6464

65+
/**
66+
* Compiles the SCSS or Sass file into {@link #getOutput()}.
67+
*
68+
* <p>With S3 asset storage on, a compiled CSS output stored under a key that covers the source
69+
* bytes, site, mode, options and build is reused, locally or restored from S3, without running
70+
* Dart Sass. A new output is written locally and uploaded. If storing it fails, the compiled
71+
* CSS is still returned and a warning is logged. A cache lease is held only while the sources
72+
* are copied and while a stored output is read, not while Dart Sass runs.
73+
*
74+
* @throws DotSecurityException if a source cannot be read by the system user
75+
* @throws DotStateException if the dotCMS state is inconsistent
76+
* @throws DotDataException if a source or its version information cannot be loaded
77+
* @throws IOException if the compile directory cannot be prepared
78+
*/
6579
@Override
6680
public void compile() throws DotSecurityException, DotStateException, DotDataException, IOException {
6781

68-
if (com.dotcms.storage.AssetStorageFeature.isEnabled()) {
69-
try (var lease = APILocator.getBinaryAssetStorageAPI().acquireCacheLease()) {
70-
compileInternal();
71-
}
72-
} else {
73-
compileInternal();
74-
}
75-
}
76-
77-
private void compileInternal() throws DotSecurityException, DotDataException, IOException {
78-
7982
String trying = inputURI.substring(0, inputURI.lastIndexOf('.')) + "." + getDefaultExtension();
8083
final File compileDir = createCompileDir( inputHost, trying, inputLive );
8184

@@ -119,18 +122,26 @@ private void compileInternal() throws DotSecurityException, DotDataException, IO
119122
final File storedCSS = com.dotcms.storage.AssetStorageFeature.isEnabled() && !sourceMap
120123
? compiledCacheFile(mainFile.getInode(), options) : null;
121124
if (storedCSS != null) {
122-
final File cached = APILocator.getBinaryAssetStorageAPI().getGeneratedFile(storedCSS);
123-
if (cached != null && (req == null || req.getParameter("recompile") == null)) {
124-
this.output = java.nio.file.Files.readAllBytes(cached.toPath());
125-
return;
125+
try (var lease = APILocator.getBinaryAssetStorageAPI().acquireCacheLease()) {
126+
final File cached = APILocator.getBinaryAssetStorageAPI().getGeneratedFile(storedCSS);
127+
if (cached != null && (req == null || req.getParameter("recompile") == null)) {
128+
this.output = java.nio.file.Files.readAllBytes(cached.toPath());
129+
return;
130+
}
126131
}
127132
}
128133
final DartSassCompiler compiler = new DartSassCompiler(options, compileTargetFile, compileDestinationFile);
129134
final Optional<String> out = compiler.compile();
130135
handleOutput(compiler.terminalOutput());
131136
this.output = out.isPresent() ? out.get().getBytes() : null;
132137
if (storedCSS != null && this.output != null && compiler.terminalOutput().exitValue() == 0) {
133-
storeCompiledOutput(storedCSS);
138+
try {
139+
storeCompiledOutput(storedCSS);
140+
} catch (final Exception e) {
141+
// Availability first: the CSS is compiled, so a storage failure must not fail the request.
142+
Logger.warnAndDebug(DotLibSassCompiler.class, "Unable to store compiled CSS for " + inputHost
143+
+ ":" + inputURI + "; serving the compiled output: " + e.getMessage(), e);
144+
}
134145
}
135146
} catch (final Exception ex) {
136147
final String errorMsg = String.format("Unable to compile SASS code in %s:%s [ live:%s ]: %s", inputHost,
@@ -158,6 +169,15 @@ private File compiledCacheFile(final String inode, final CompilerOptions options
158169
inode.charAt(0) + "/" + inode.charAt(1) + "/" + inode + "/dotGenerated_css_" + hash + ".css");
159170
}
160171

172+
/**
173+
* Writes the compiled CSS atomically to its local {@code dotGenerated} path and uploads it.
174+
* If the upload fails, the local file is kept: later requests on this node serve it, and
175+
* eviction never removes it because it has no durable copy.
176+
*
177+
* @param target the local path of the compiled CSS
178+
* @throws IOException if the local file cannot be written
179+
* @throws DotDataException if the upload fails
180+
*/
161181
private void storeCompiledOutput(final File target) throws IOException, DotDataException {
162182
final java.nio.file.Path path = target.toPath();
163183
java.nio.file.Files.createDirectories(path.getParent());
@@ -170,17 +190,7 @@ private void storeCompiledOutput(final File target) throws IOException, DotDataE
170190
} catch (java.nio.file.AtomicMoveNotSupportedException e) {
171191
java.nio.file.Files.move(temporary, path, java.nio.file.StandardCopyOption.REPLACE_EXISTING);
172192
}
173-
try {
174-
APILocator.getBinaryAssetStorageAPI().storeGeneratedFile(target);
175-
} catch (DotDataException e) {
176-
// Failed publication must not leave a warm-only result that is never uploaded on retry.
177-
try {
178-
java.nio.file.Files.deleteIfExists(path);
179-
} catch (IOException cleanup) {
180-
e.addSuppressed(cleanup);
181-
}
182-
throw e;
183-
}
193+
APILocator.getBinaryAssetStorageAPI().storeGeneratedFile(target);
184194
} finally {
185195
java.nio.file.Files.deleteIfExists(temporary);
186196
}

‎dotCMS/src/main/java/com/dotcms/rendering/velocity/services/DotResourceLoader.java‎

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,14 @@
22

33

44
import com.dotmarketing.business.CacheLocator;
5+
import com.dotmarketing.exception.DotDataException;
56
import com.dotmarketing.util.Config;
67
import com.dotmarketing.util.Logger;
78
import com.dotmarketing.util.UtilMethods;
89
import java.io.InputStream;
910
import org.apache.commons.collections.ExtendedProperties;
1011
import org.apache.velocity.exception.ResourceNotFoundException;
12+
import org.apache.velocity.exception.VelocityException;
1113
import org.apache.velocity.runtime.resource.Resource;
1214
import org.apache.velocity.runtime.resource.loader.ResourceLoader;
1315

@@ -22,6 +24,19 @@ public DotResourceLoader() {
2224
}
2325

2426

27+
/**
28+
* Opens the Velocity source for a resource path, choosing the loader by the path's type.
29+
*
30+
* <p>A failure normally records a cache miss and is reported as a
31+
* {@link ResourceNotFoundException}. With S3 asset storage on, a {@link DotDataException}
32+
* from a loader that reads a stored file (VTL, macro, legacy VL and include files) means the
33+
* file could not be restored from storage. It is reported as a plain {@link VelocityException}
34+
* and no miss is recorded, so the next render tries the storage again.
35+
*
36+
* @param filePath the Velocity resource path
37+
* @return the resource source
38+
* @throws ResourceNotFoundException if the path is empty or the resource cannot be loaded
39+
*/
2540
@Override
2641
public InputStream getResourceStream(final String filePath) throws ResourceNotFoundException {
2742
if (!UtilMethods.isSet(filePath)) {
@@ -76,8 +91,8 @@ public InputStream getResourceStream(final String filePath) throws ResourceNotFo
7691
} catch (Exception e) {
7792
Logger.warn(this, "filePath: " + filePath + ", msg:" + e.getMessage(), e);
7893
if (com.dotcms.storage.AssetStorageFeature.isEnabled()
79-
&& e instanceof com.dotmarketing.exception.DotDataException) {
80-
throw new org.apache.velocity.exception.VelocityException("Unable to load stored resource: " + key.path, e);
94+
&& e instanceof DotDataException && readsStoredFile(key.type)) {
95+
throw new VelocityException("Unable to load stored resource: " + key.path, e);
8196
}
8297
CacheLocator.getVeloctyResourceCache().addMiss(key.path);
8398
throw new ResourceNotFoundException("Cannot parse velocity file : " + key.path, e);
@@ -86,6 +101,29 @@ public InputStream getResourceStream(final String filePath) throws ResourceNotFo
86101

87102
}
88103

104+
/**
105+
* Tells whether a resource type is loaded from a file that S3 asset storage may have to
106+
* restore. These are the types {@link #getResourceStream} hands to {@link VTLLoader} and
107+
* {@link IncludeLoader}; the other loaders build their source from the database.
108+
*
109+
* @param type the resource type
110+
* @return {@code true} for VTL, macro, legacy VL and include files
111+
*/
112+
private static boolean readsStoredFile(final VelocityType type) {
113+
switch (type) {
114+
case CONTAINER:
115+
case TEMPLATE:
116+
case CONTENT:
117+
case FIELD:
118+
case CONTENT_TYPE:
119+
case SITE:
120+
case HTMLPAGE:
121+
return false;
122+
default:
123+
return true;
124+
}
125+
}
126+
89127
/*
90128
* (non-Javadoc)
91129
*

‎dotCMS/src/main/java/com/dotcms/rendering/velocity/services/IncludeLoader.java‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,24 @@ private Set<String> buildAllowedExtensions() {
5858
}
5959

6060

61+
/**
62+
* Checks that an included file may be served: it must be under the Velocity root or the asset
63+
* root and have an allowed extension.
64+
*
65+
* <p>With S3 asset storage on, a missing file under the asset root passes this check so that
66+
* it can be restored from storage. A missing file anywhere else is reported as not found, as
67+
* with the flag off, rather than logged as a possible attack.
68+
*
69+
* @param filePath the file path from the Velocity resource key
70+
* @return the file to serve
71+
* @throws IOException if a canonical path cannot be resolved
72+
* @throws ResourceNotFoundException if the file is missing or not allowed
73+
*/
6174
private File allowedToServe(final String filePath) throws IOException {
6275
final File fileToServe = new File(filePath);
6376
final boolean s3 = com.dotcms.storage.AssetStorageFeature.isEnabled();
64-
if (!fileToServe.exists() && !s3) {
77+
if (!fileToServe.exists()
78+
&& (!s3 || !fileToServe.getCanonicalFile().toPath().startsWith(allowedPaths[1]))) {
6579
throw new ResourceNotFoundException(CANNOT_FIND_RESOURCE + filePath);
6680
}
6781
String canonicalPath = fileToServe.getCanonicalPath();

‎dotCMS/src/main/java/com/dotcms/rendering/velocity/services/VTLLoader.java‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,34 @@ private void init() {
8080

8181
}
8282

83+
/**
84+
* Tells whether a file is under the asset root, the only place S3 asset storage can restore
85+
* a missing file from.
86+
*
87+
* @param file the file to check
88+
* @return {@code true} if its canonical path is under the canonical asset root
89+
* @throws IOException if a canonical path cannot be resolved
90+
*/
91+
private static boolean underAssetRoot(final File file) throws IOException {
92+
final String assetRoot = ConfigUtils.getAssetPath();
93+
return UtilMethods.isSet(assetRoot) && file.getCanonicalFile().toPath()
94+
.startsWith(new File(assetRoot).getCanonicalFile().toPath());
95+
}
96+
97+
/**
98+
* Opens a VTL, macro or legacy VL file from the Velocity root, the dynamic content path or
99+
* the asset directories, refusing paths outside them.
100+
*
101+
* <p>With S3 asset storage on, the file is opened through the binary asset API, so a file
102+
* whose local copy was evicted is restored first. A missing file outside the asset root
103+
* cannot be restored and is reported as not found, as with the flag off.
104+
*
105+
* @param filePath the file path from the Velocity resource key
106+
* @return the file contents
107+
* @throws IOException if the file cannot be read
108+
* @throws DotDataException if restoring the file from storage fails
109+
* @throws ResourceNotFoundException if the file is missing or outside the allowed roots
110+
*/
83111
InputStream streamFile(String filePath) throws IOException, DotDataException {
84112
final boolean s3 = com.dotcms.storage.AssetStorageFeature.isEnabled();
85113
boolean serveFile = false;
@@ -97,7 +125,8 @@ InputStream streamFile(String filePath) throws IOException, DotDataException {
97125
if (!f.exists()) {
98126
f = new java.io.File(filePath);
99127
}
100-
if (!f.exists() && !s3) {
128+
// A missing file can only be restored from storage when it is under the asset root.
129+
if (!f.exists() && (!s3 || !underAssetRoot(f))) {
101130
throw new ResourceNotFoundException("cannot find resource");
102131
}
103132
String canon = f.getCanonicalPath();

0 commit comments

Comments
 (0)