From bd6156328b2e4f85f85576d1079b83c7d2323a7e Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 19 May 2026 15:18:57 -0400 Subject: [PATCH 01/12] feat(request-cost): publish aggregate token telemetry to external endpoint Add a scheduled push of the per-window + lifetime request-cost (token) counters to an external REST collector. The work runs on the existing monitor scheduler tick and is dispatched to DotConcurrentFactory's submitter so HTTP latency never blocks the monitor thread. - RequestCostSnapshot: immutable JSON payload (clusterId, environmentId, timestamp, window + lifetime request/token counters + averages). - RequestCostPublisher: @ApplicationScoped CDI bean. Activates implicitly when REQUEST_COST_PUSH_URL and REQUEST_COST_PUSH_TOKEN are both set. POSTs via CircuitBreakerUrl with bearer auth; transport failures are warnEvery-rate limited and the snapshot is dropped (observational telemetry). - RequestCostApiImpl.logRequestCost: builds a snapshot from the same counters it already logs, calls publisher on each tick. - Tests: lock the on-the-wire JSON shape (10 fields exact) and the enable gate. Cluster ID via ClusterFactory.getClusterId(); environment ID via ConfigUtils.getServerId(). Both wrapped in Try with "unknown" fallback. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../com/dotcms/cost/RequestCostApiImpl.java | 25 ++++- .../com/dotcms/cost/RequestCostPublisher.java | 100 ++++++++++++++++++ .../com/dotcms/cost/RequestCostSnapshot.java | 49 +++++++++ .../dotcms/cost/RequestCostPublisherTest.java | 77 ++++++++++++++ .../dotcms/cost/RequestCostSnapshotTest.java | 79 ++++++++++++++ 5 files changed, 327 insertions(+), 3 deletions(-) create mode 100644 dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java create mode 100644 dotCMS/src/main/java/com/dotcms/cost/RequestCostSnapshot.java create mode 100644 dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java create mode 100644 dotCMS/src/test/java/com/dotcms/cost/RequestCostSnapshotTest.java diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java index 4333d8d04ea..be5c00b0d3d 100644 --- a/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java @@ -4,11 +4,15 @@ import com.dotcms.auth.providers.jwt.services.JsonWebTokenAuthCredentialProcessorImpl; import com.dotcms.cdi.CDIUtils; import com.dotcms.cost.RequestPrices.Price; +import com.dotcms.enterprise.cluster.ClusterFactory; import com.dotmarketing.util.Config; +import com.dotmarketing.util.ConfigUtils; import com.dotmarketing.util.Logger; import com.liferay.portal.model.User; import com.liferay.portal.util.PortalUtil; +import io.vavr.control.Try; import java.lang.reflect.Method; +import java.time.Instant; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -43,6 +47,7 @@ public class RequestCostApiImpl implements RequestCostApi { private double requestCostDenominator = 1.0d; private final LeakyTokenBucket bucket = CDIUtils.getBeanThrows(LeakyTokenBucket.class); + private final RequestCostPublisher publisher = CDIUtils.getBeanThrows(RequestCostPublisher.class); public RequestCostApiImpl() { enableForTests = Optional.empty(); @@ -95,9 +100,9 @@ private void logRequestCost() { : totalCostTotal / totalRequestsTotal; - Logger.info("REQUEST COST MONITOR >", + Logger.info("REQUEST TOKEN MONITOR >", String.format( - "Last %ds: Reqs: %d, Cost: %.2f, Avg Cost: %.2f | Totals: Reqs: %d, Cost: %.2f, Avg Cost: %.2f", + "Last %ds: Reqs: %d, Tokens: %.2f, Avg Tokens: %.2f | Totals: Reqs: %d, Tokens: %.2f, Avg Tokens: %.2f", requestCostTimeWindowSeconds, totalRequestsForDuration, totalCostForDuration, @@ -105,8 +110,22 @@ private void logRequestCost() { totalRequestsTotal, totalCostTotal, costPerRequestTotal)); + + if (publisher.isEnabled()) { + publisher.publish(new RequestCostSnapshot( + Try.of(ClusterFactory::getClusterId).getOrElse("unknown"), + Try.of(ConfigUtils::getServerId).getOrElse("unknown"), + Instant.now().toString(), + requestCostTimeWindowSeconds, + totalRequestsForDuration, + totalCostForDuration, + costPerRequestForDuration, + totalRequestsTotal, + totalCostTotal, + costPerRequestTotal)); + } } catch (Exception e) { - Logger.warnAndDebug(this.getClass(), "Error logging request cost:" + e.getMessage(), e); + Logger.warnAndDebug(this.getClass(), "Error logging request tokens:" + e.getMessage(), e); } } diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java new file mode 100644 index 00000000000..90ddea90a0d --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java @@ -0,0 +1,100 @@ +package com.dotcms.cost; + +import com.dotcms.concurrent.DotConcurrentFactory; +import com.dotcms.http.CircuitBreakerUrl; +import com.dotmarketing.util.Config; +import com.dotmarketing.util.Logger; +import com.dotmarketing.util.UtilMethods; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.util.HashMap; +import java.util.Map; +import javax.enterprise.context.ApplicationScoped; + +/** + * Ships {@link RequestCostSnapshot} payloads to an external REST endpoint on each tick of + * {@link RequestCostApiImpl#logRequestCost()}. + * + *

Activates implicitly when both {@code REQUEST_COST_PUSH_URL} and + * {@code REQUEST_COST_PUSH_TOKEN} are set. Failures are rate-limited warnings and the snapshot + * is dropped — this is observational telemetry, not durable accounting.

+ * + *

Config keys: + *

+ *

+ */ +@ApplicationScoped +public class RequestCostPublisher { + + private static final ObjectMapper MAPPER = new ObjectMapper(); + private static final long FAIL_LOG_INTERVAL_MS = 60 * 10 * 1000L; + + public boolean isEnabled() { + return UtilMethods.isSet(getUrl()) && UtilMethods.isSet(getToken()); + } + + private String getUrl() { + return Config.getStringProperty("REQUEST_COST_PUSH_URL", null); + } + + private String getToken() { + return Config.getStringProperty("REQUEST_COST_PUSH_TOKEN", null); + } + + private long getTimeoutMs() { + return Config.getLongProperty("REQUEST_COST_PUSH_TIMEOUT_MS", 5_000L); + } + + /** + * Submits the snapshot to {@link DotConcurrentFactory}'s default submitter so the HTTP POST + * never blocks the request-cost monitor scheduler. Returns immediately. Transport errors are + * logged at most once per minute and the snapshot is dropped. + */ + public void publish(final RequestCostSnapshot snapshot) { + if (!isEnabled()) { + return; + } + DotConcurrentFactory.getInstance().getSubmitter().submit(() -> post(snapshot)); + } + + private void post(final RequestCostSnapshot snapshot) { + final String url = getUrl(); + if (!UtilMethods.isSet(url)) { + return; + } + try { + final Map headers = new HashMap<>(); + headers.put("Content-Type", "application/json"); + final String token = getToken(); + if (UtilMethods.isSet(token)) { + headers.put("Authorization", "Bearer " + token); + } + + final CircuitBreakerUrl call = CircuitBreakerUrl.builder() + .setMethod(CircuitBreakerUrl.Method.POST) + .setUrl(url) + .setHeaders(headers) + .setRawData(MAPPER.writeValueAsString(snapshot)) + .setTimeout(getTimeoutMs()) + .setThrowWhenError(false) + .build(); + + call.doString(); + final int response = call.response(); + if (!CircuitBreakerUrl.isSuccessResponse(response)) { + Logger.warnEvery(this.getClass(), + "REQUEST_COST_PUSH_FAIL", + "Request cost push to " + url + " returned HTTP " + response, + (int) FAIL_LOG_INTERVAL_MS); + } + } catch (final Exception e) { + Logger.warnEvery(this.getClass(), + "REQUEST_COST_PUSH_ERR", + "Request cost push to " + url + " failed: " + e.getMessage(), + (int) FAIL_LOG_INTERVAL_MS); + } + } +} diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostSnapshot.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostSnapshot.java new file mode 100644 index 00000000000..b294eedd55e --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostSnapshot.java @@ -0,0 +1,49 @@ +package com.dotcms.cost; + +import com.fasterxml.jackson.annotation.JsonAutoDetect; +import com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility; + +/** + * Immutable payload shipped to the external request-cost (a.k.a. request token) collection + * endpoint on each scheduled tick. One snapshot = one point in the time series. + */ +@JsonAutoDetect( + fieldVisibility = Visibility.ANY, + getterVisibility = Visibility.NONE, + isGetterVisibility = Visibility.NONE) +public final class RequestCostSnapshot { + + public final String clusterId; + public final String environmentId; + public final String timestamp; + public final int windowSeconds; + public final long windowRequests; + public final double windowTokens; + public final double windowAvgTokensPerRequest; + public final long lifetimeRequests; + public final double lifetimeTokens; + public final double lifetimeAvgTokensPerRequest; + + public RequestCostSnapshot( + final String clusterId, + final String environmentId, + final String timestamp, + final int windowSeconds, + final long windowRequests, + final double windowTokens, + final double windowAvgTokensPerRequest, + final long lifetimeRequests, + final double lifetimeTokens, + final double lifetimeAvgTokensPerRequest) { + this.clusterId = clusterId; + this.environmentId = environmentId; + this.timestamp = timestamp; + this.windowSeconds = windowSeconds; + this.windowRequests = windowRequests; + this.windowTokens = windowTokens; + this.windowAvgTokensPerRequest = windowAvgTokensPerRequest; + this.lifetimeRequests = lifetimeRequests; + this.lifetimeTokens = lifetimeTokens; + this.lifetimeAvgTokensPerRequest = lifetimeAvgTokensPerRequest; + } +} diff --git a/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java b/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java new file mode 100644 index 00000000000..b50cdd52870 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java @@ -0,0 +1,77 @@ +package com.dotcms.cost; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import com.dotcms.UnitTestBase; +import com.dotmarketing.util.Config; +import org.junit.After; +import org.junit.Test; + +/** + * Unit tests for {@link RequestCostPublisher}. Focuses on the enable gate and the + * disabled-is-a-no-op guarantee. The actual HTTP POST is covered indirectly by + * {@link RequestCostSnapshotTest} (payload shape) and would need a localhost HTTP server plus + * {@code ALLOW_ACCESS_TO_PRIVATE_SUBNETS=true} set before {@code CircuitBreakerUrl}'s static + * initializer fires — not a stable unit-test surface. + */ +public class RequestCostPublisherTest extends UnitTestBase { + + private final RequestCostPublisher publisher = new RequestCostPublisher(); + + @After + public void clearConfig() { + Config.setProperty("REQUEST_COST_PUSH_URL", null); + Config.setProperty("REQUEST_COST_PUSH_TOKEN", null); + } + + private RequestCostSnapshot anySnapshot() { + return new RequestCostSnapshot( + "c", "e", "2026-05-19T00:00:00Z", + 60, 0L, 0d, 0d, 0L, 0d, 0d); + } + + @Test + public void test_isEnabled_returnsFalse_whenUrlAndTokenAreMissing() { + // Given — clearConfig in @After ensures both are null + + // When / Then + assertFalse("publisher must default to off", publisher.isEnabled()); + } + + @Test + public void test_isEnabled_returnsFalse_whenOnlyUrlIsSet() { + // Given + Config.setProperty("REQUEST_COST_PUSH_URL", "https://example.com/cost"); + + // When / Then + assertFalse("url alone must not enable the publisher", publisher.isEnabled()); + } + + @Test + public void test_isEnabled_returnsFalse_whenOnlyTokenIsSet() { + // Given + Config.setProperty("REQUEST_COST_PUSH_TOKEN", "secret"); + + // When / Then + assertFalse("token alone must not enable the publisher", publisher.isEnabled()); + } + + @Test + public void test_isEnabled_returnsTrue_whenUrlAndTokenAreBothSet() { + // Given + Config.setProperty("REQUEST_COST_PUSH_URL", "https://example.com/cost"); + Config.setProperty("REQUEST_COST_PUSH_TOKEN", "secret"); + + // When / Then + assertTrue("publisher must activate when both keys are present", publisher.isEnabled()); + } + + @Test + public void test_publish_whenDisabled_isANoOp() { + // Given — both keys absent + + // When / Then — must not throw, must not submit anything to the executor + publisher.publish(anySnapshot()); + } +} diff --git a/dotCMS/src/test/java/com/dotcms/cost/RequestCostSnapshotTest.java b/dotCMS/src/test/java/com/dotcms/cost/RequestCostSnapshotTest.java new file mode 100644 index 00000000000..c7ab37715a1 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/cost/RequestCostSnapshotTest.java @@ -0,0 +1,79 @@ +package com.dotcms.cost; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import com.dotcms.UnitTestBase; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.Test; + +/** + * Unit tests for {@link RequestCostSnapshot}. The snapshot is the on-the-wire payload, so the + * Jackson field-visibility config is the bug surface most likely to break silently — these tests + * lock the JSON shape down. + */ +public class RequestCostSnapshotTest extends UnitTestBase { + + private static final ObjectMapper MAPPER = new ObjectMapper(); + + private RequestCostSnapshot sample() { + return new RequestCostSnapshot( + "cluster-1", + "server-7", + "2026-05-19T18:48:00Z", + 60, + 1234L, + 5678.5d, + 4.6d, + 999_999L, + 12_345_678.25d, + 12.35d); + } + + @Test + public void test_serialization_includesAllExpectedFields() throws Exception { + // When + final JsonNode json = MAPPER.readTree(MAPPER.writeValueAsString(sample())); + + // Then + assertTrue("missing clusterId", json.has("clusterId")); + assertTrue("missing environmentId", json.has("environmentId")); + assertTrue("missing timestamp", json.has("timestamp")); + assertTrue("missing windowSeconds", json.has("windowSeconds")); + assertTrue("missing windowRequests", json.has("windowRequests")); + assertTrue("missing windowTokens", json.has("windowTokens")); + assertTrue("missing windowAvgTokensPerRequest", json.has("windowAvgTokensPerRequest")); + assertTrue("missing lifetimeRequests", json.has("lifetimeRequests")); + assertTrue("missing lifetimeTokens", json.has("lifetimeTokens")); + assertTrue("missing lifetimeAvgTokensPerRequest", json.has("lifetimeAvgTokensPerRequest")); + } + + @Test + public void test_serialization_preservesValues() throws Exception { + // When + final JsonNode json = MAPPER.readTree(MAPPER.writeValueAsString(sample())); + + // Then + assertEquals("cluster-1", json.get("clusterId").asText()); + assertEquals("server-7", json.get("environmentId").asText()); + assertEquals("2026-05-19T18:48:00Z", json.get("timestamp").asText()); + assertEquals(60, json.get("windowSeconds").asInt()); + assertEquals(1234L, json.get("windowRequests").asLong()); + assertEquals(5678.5d, json.get("windowTokens").asDouble(), 0.0001d); + assertEquals(4.6d, json.get("windowAvgTokensPerRequest").asDouble(), 0.0001d); + assertEquals(999_999L, json.get("lifetimeRequests").asLong()); + assertEquals(12_345_678.25d, json.get("lifetimeTokens").asDouble(), 0.0001d); + assertEquals(12.35d, json.get("lifetimeAvgTokensPerRequest").asDouble(), 0.0001d); + } + + @Test + public void test_serialization_emitsExactlyTenFields() throws Exception { + // When + final JsonNode json = MAPPER.readTree(MAPPER.writeValueAsString(sample())); + + // Then — guard against accidental leakage of internal fields if someone adds private + // helpers later without updating the @JsonAutoDetect visibility + assertEquals("unexpected fields on the wire", 10, json.size()); + } +} From dfafca18ab9e313065b2a33f0404124d8647f585 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 19 May 2026 15:28:06 -0400 Subject: [PATCH 02/12] fix(request-cost): address Claude AI review feedback on PR #35749 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Align javadoc with the 10-minute fail-log throttle (was stated as "once per minute" but the constant is 600_000ms). - Change FAIL_LOG_INTERVAL_MS from long to int and drop the (int) casts — the value fits in int and Logger.warnEvery takes int, so the cast was theoretical overflow waiting to happen. - Sanitize the push URL before logging: strip RFC-3986 userinfo so a misconfigured REQUEST_COST_PUSH_URL=https://user:pass@host/... doesn't leak credentials into every failure log line. - Add tests for the sanitizer (with/without userinfo) and a regression test that Config.setProperty(key, null) actually unsets the property (so the enable gate can't silently flip on if Apache Commons ever stringifies null). 11/11 tests passing locally. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../com/dotcms/cost/RequestCostPublisher.java | 36 ++++++++++++--- .../dotcms/cost/RequestCostPublisherTest.java | 46 +++++++++++++++++++ 2 files changed, 76 insertions(+), 6 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java index 90ddea90a0d..2154a8c2302 100644 --- a/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java @@ -6,6 +6,7 @@ import com.dotmarketing.util.Logger; import com.dotmarketing.util.UtilMethods; import com.fasterxml.jackson.databind.ObjectMapper; +import java.net.URI; import java.util.HashMap; import java.util.Map; import javax.enterprise.context.ApplicationScoped; @@ -30,7 +31,7 @@ public class RequestCostPublisher { private static final ObjectMapper MAPPER = new ObjectMapper(); - private static final long FAIL_LOG_INTERVAL_MS = 60 * 10 * 1000L; + private static final int FAIL_LOG_INTERVAL_MS = 10 * 60 * 1000; public boolean isEnabled() { return UtilMethods.isSet(getUrl()) && UtilMethods.isSet(getToken()); @@ -51,7 +52,7 @@ private long getTimeoutMs() { /** * Submits the snapshot to {@link DotConcurrentFactory}'s default submitter so the HTTP POST * never blocks the request-cost monitor scheduler. Returns immediately. Transport errors are - * logged at most once per minute and the snapshot is dropped. + * logged at most once every 10 minutes and the snapshot is dropped. */ public void publish(final RequestCostSnapshot snapshot) { if (!isEnabled()) { @@ -87,14 +88,37 @@ private void post(final RequestCostSnapshot snapshot) { if (!CircuitBreakerUrl.isSuccessResponse(response)) { Logger.warnEvery(this.getClass(), "REQUEST_COST_PUSH_FAIL", - "Request cost push to " + url + " returned HTTP " + response, - (int) FAIL_LOG_INTERVAL_MS); + "Request cost push to " + sanitizeUrlForLog(url) + " returned HTTP " + response, + FAIL_LOG_INTERVAL_MS); } } catch (final Exception e) { Logger.warnEvery(this.getClass(), "REQUEST_COST_PUSH_ERR", - "Request cost push to " + url + " failed: " + e.getMessage(), - (int) FAIL_LOG_INTERVAL_MS); + "Request cost push to " + sanitizeUrlForLog(url) + " failed: " + e.getMessage(), + FAIL_LOG_INTERVAL_MS); + } + } + + /** + * Strips RFC-3986 userinfo (the {@code user:pass@} segment) from a URL before logging so a + * misconfigured {@code REQUEST_COST_PUSH_URL=https://user:secret@host/...} doesn't leak the + * credential into every failure log line. + */ + static String sanitizeUrlForLog(final String url) { + if (!UtilMethods.isSet(url)) { + return url; + } + try { + final URI uri = URI.create(url); + if (uri.getUserInfo() == null) { + return url; + } + final URI safe = new URI( + uri.getScheme(), null, uri.getHost(), uri.getPort(), + uri.getPath(), uri.getQuery(), uri.getFragment()); + return safe.toString(); + } catch (final Exception ignored) { + return ""; } } } diff --git a/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java b/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java index b50cdd52870..edaca738a0b 100644 --- a/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java +++ b/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java @@ -1,6 +1,8 @@ package com.dotcms.cost; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import com.dotcms.UnitTestBase; @@ -74,4 +76,48 @@ public void test_publish_whenDisabled_isANoOp() { // When / Then — must not throw, must not submit anything to the executor publisher.publish(anySnapshot()); } + + /** + * Verifies that {@link Config#setProperty(String, Object) Config.setProperty(key, null)} + * actually unsets the property — i.e. the enable gate sees null, not the literal string + * {@code "null"}. If this ever regresses, {@link UtilMethods#isSet(Object)} would return true + * for {@code "null"} and the gate would silently flip on. + */ + @Test + public void test_clearConfig_actuallyRemovesProperties() { + // Given — both keys set then cleared + Config.setProperty("REQUEST_COST_PUSH_URL", "https://example.com/cost"); + Config.setProperty("REQUEST_COST_PUSH_TOKEN", "secret"); + Config.setProperty("REQUEST_COST_PUSH_URL", null); + Config.setProperty("REQUEST_COST_PUSH_TOKEN", null); + + // When + final String url = Config.getStringProperty("REQUEST_COST_PUSH_URL", null); + final String token = Config.getStringProperty("REQUEST_COST_PUSH_TOKEN", null); + + // Then — must be null, not the literal string "null" + assertNull("URL must be unset, not stringified", url); + assertNull("token must be unset, not stringified", token); + assertFalse("publisher must observe the cleared state", publisher.isEnabled()); + } + + @Test + public void test_sanitizeUrlForLog_stripsUserinfo() { + // Given / When + final String sanitized = RequestCostPublisher.sanitizeUrlForLog( + "https://user:secret@host.example.com/cost?x=1"); + + // Then — no credentials leak into log lines + assertEquals("https://host.example.com/cost?x=1", sanitized); + } + + @Test + public void test_sanitizeUrlForLog_passesPlainUrlThrough() { + // Given / When + final String sanitized = RequestCostPublisher.sanitizeUrlForLog( + "https://host.example.com/cost"); + + // Then + assertEquals("https://host.example.com/cost", sanitized); + } } From 81aff381739d36779990d7e634093a4a3d8a3766 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 19 May 2026 15:36:30 -0400 Subject: [PATCH 03/12] =?UTF-8?q?fix(request-cost):=20second=20round=20of?= =?UTF-8?q?=20review=20fixes=20=E2=80=94=20continuous=20telemetry,=20deter?= =?UTF-8?q?ministic=20timestamps,=20header=20injection=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the second Claude AI review of PR #35749. No-blocker review, but the behavioral and operability items are worth landing: - Continuous time series. The skipZeroRequests gate now only suppresses the log line on consecutive idle windows; the publisher still emits a zero-snapshot every tick when enabled. This makes "idle cluster" and "agent down" graphable side-by-side on the collector. - Deterministic timestamp. Truncate to seconds (Instant.now().truncatedTo(SECONDS)) so the wire format is stable ISO-8601 seconds, matching the PR description and what most TSDBs index on. - Header injection guard. New sanitizeHeaderValue() strips CR/LF + trims before the bearer token hits the Authorization header. Tiny defense-in-depth — only an operator can set the token, but CRLF in a misconfigured value would otherwise be HTTP header injection. - Broaden @After cleanup to include REQUEST_COST_PUSH_TIMEOUT_MS so future tests don't bleed state. Tests: 12/12 passing locally. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../com/dotcms/cost/RequestCostApiImpl.java | 50 ++++++++++--------- .../com/dotcms/cost/RequestCostPublisher.java | 11 +++- .../dotcms/cost/RequestCostPublisherTest.java | 12 +++++ 3 files changed, 49 insertions(+), 24 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java index be5c00b0d3d..f46df0f5520 100644 --- a/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java @@ -13,6 +13,7 @@ import io.vavr.control.Try; import java.lang.reflect.Method; import java.time.Instant; +import java.time.temporal.ChronoUnit; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -82,40 +83,43 @@ private void logRequestCost() { return; } - long totalRequestsForDuration = this.requestCountForWindow.sumThenReset(); - double totalCostForDuration = this.requestCostForWindow.sumThenReset() / getRequestCostDenominator(); - double costPerRequestForDuration = totalRequestsForDuration == 0 + final long totalRequestsForDuration = this.requestCountForWindow.sumThenReset(); + final double totalCostForDuration = this.requestCostForWindow.sumThenReset() / getRequestCostDenominator(); + final double costPerRequestForDuration = totalRequestsForDuration == 0 ? 0 : totalCostForDuration / totalRequestsForDuration; - if (totalRequestsForDuration == 0 && skipZeroRequests) { - return; - } - skipZeroRequests = totalRequestsForDuration == 0; - - long totalRequestsTotal = requestCountTotal.longValue(); - double totalCostTotal = requestCostTotal.longValue() / getRequestCostDenominator(); - double costPerRequestTotal = totalRequestsTotal == 0 + final long totalRequestsTotal = requestCountTotal.longValue(); + final double totalCostTotal = requestCostTotal.longValue() / getRequestCostDenominator(); + final double costPerRequestTotal = totalRequestsTotal == 0 ? 0 : totalCostTotal / totalRequestsTotal; - - Logger.info("REQUEST TOKEN MONITOR >", - String.format( - "Last %ds: Reqs: %d, Tokens: %.2f, Avg Tokens: %.2f | Totals: Reqs: %d, Tokens: %.2f, Avg Tokens: %.2f", - requestCostTimeWindowSeconds, - totalRequestsForDuration, - totalCostForDuration, - costPerRequestForDuration, - totalRequestsTotal, - totalCostTotal, - costPerRequestTotal)); + // The log line is throttled on consecutive idle windows so dev consoles stay quiet. + // The publisher is NOT throttled — telemetry must emit a point every tick so an idle + // cluster and a downed cluster are distinguishable on the receiving side. + final boolean idleWindow = totalRequestsForDuration == 0; + final boolean suppressLog = idleWindow && skipZeroRequests; + skipZeroRequests = idleWindow; + + if (!suppressLog) { + Logger.info("REQUEST TOKEN MONITOR >", + String.format( + "Last %ds: Reqs: %d, Tokens: %.2f, Avg Tokens: %.2f | Totals: Reqs: %d, Tokens: %.2f, Avg Tokens: %.2f", + requestCostTimeWindowSeconds, + totalRequestsForDuration, + totalCostForDuration, + costPerRequestForDuration, + totalRequestsTotal, + totalCostTotal, + costPerRequestTotal)); + } if (publisher.isEnabled()) { publisher.publish(new RequestCostSnapshot( Try.of(ClusterFactory::getClusterId).getOrElse("unknown"), Try.of(ConfigUtils::getServerId).getOrElse("unknown"), - Instant.now().toString(), + Instant.now().truncatedTo(ChronoUnit.SECONDS).toString(), requestCostTimeWindowSeconds, totalRequestsForDuration, totalCostForDuration, diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java index 2154a8c2302..44311be8c5e 100644 --- a/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java @@ -69,7 +69,7 @@ private void post(final RequestCostSnapshot snapshot) { try { final Map headers = new HashMap<>(); headers.put("Content-Type", "application/json"); - final String token = getToken(); + final String token = sanitizeHeaderValue(getToken()); if (UtilMethods.isSet(token)) { headers.put("Authorization", "Bearer " + token); } @@ -99,6 +99,15 @@ private void post(final RequestCostSnapshot snapshot) { } } + /** + * Strips CR/LF and surrounding whitespace from a header value. A misconfigured + * {@code REQUEST_COST_PUSH_TOKEN} containing CRLF would otherwise enable HTTP header + * injection — low-risk since only operators set this, but cheap to harden. + */ + static String sanitizeHeaderValue(final String value) { + return value == null ? null : value.replace("\r", "").replace("\n", "").trim(); + } + /** * Strips RFC-3986 userinfo (the {@code user:pass@} segment) from a URL before logging so a * misconfigured {@code REQUEST_COST_PUSH_URL=https://user:secret@host/...} doesn't leak the diff --git a/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java b/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java index edaca738a0b..80d832eee4c 100644 --- a/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java +++ b/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java @@ -25,6 +25,7 @@ public class RequestCostPublisherTest extends UnitTestBase { public void clearConfig() { Config.setProperty("REQUEST_COST_PUSH_URL", null); Config.setProperty("REQUEST_COST_PUSH_TOKEN", null); + Config.setProperty("REQUEST_COST_PUSH_TIMEOUT_MS", null); } private RequestCostSnapshot anySnapshot() { @@ -120,4 +121,15 @@ public void test_sanitizeUrlForLog_passesPlainUrlThrough() { // Then assertEquals("https://host.example.com/cost", sanitized); } + + @Test + public void test_sanitizeHeaderValue_stripsCrlfAndTrims() { + // CRLF in a header value would otherwise enable HTTP header injection + assertEquals("legit-token", RequestCostPublisher.sanitizeHeaderValue("legit-token")); + assertEquals("legit-token", RequestCostPublisher.sanitizeHeaderValue(" legit-token ")); + assertEquals("evilXInjected: value", + RequestCostPublisher.sanitizeHeaderValue("evil\r\nXInjected: value")); + assertEquals("token", RequestCostPublisher.sanitizeHeaderValue("token\n")); + assertNull(RequestCostPublisher.sanitizeHeaderValue(null)); + } } From b2c6fcf9ca8211546ce7f77934c1fc5e97702f94 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 19 May 2026 15:42:50 -0400 Subject: [PATCH 04/12] fix(request-cost): close whitespace-token auth-strip gap + clearer fail log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third round of Claude AI review on PR #35749. The one merge-worthy item: - Gate disagreement bug. isEnabled() checked the raw token via isSet(), but post() sanitized (strip CRLF + trim) before putting it in the Authorization header. A token of " \r\n " would pass the gate and result in an *unauthenticated* POST to the collector — strictly worse than staying off. Move sanitization into the gate so this can't happen, and add a regression test for whitespace/CRLF-only tokens. - Clearer failure log. CircuitBreakerUrl can return without setting a response code (circuit open, transport error before response). Check isProcessed() first so we emit "did not complete" rather than "returned HTTP -1". 13/13 tests passing locally. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../com/dotcms/cost/RequestCostPublisher.java | 11 ++++++++++- .../dotcms/cost/RequestCostPublisherTest.java | 17 +++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java index 44311be8c5e..7885dde333a 100644 --- a/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java @@ -34,7 +34,9 @@ public class RequestCostPublisher { private static final int FAIL_LOG_INTERVAL_MS = 10 * 60 * 1000; public boolean isEnabled() { - return UtilMethods.isSet(getUrl()) && UtilMethods.isSet(getToken()); + // Use the sanitized token in the gate so a whitespace-or-CRLF-only token + // doesn't activate the publisher and cause an unauthenticated POST. + return UtilMethods.isSet(getUrl()) && UtilMethods.isSet(sanitizeHeaderValue(getToken())); } private String getUrl() { @@ -84,6 +86,13 @@ private void post(final RequestCostSnapshot snapshot) { .build(); call.doString(); + if (!call.isProcessed()) { + Logger.warnEvery(this.getClass(), + "REQUEST_COST_PUSH_FAIL", + "Request cost push to " + sanitizeUrlForLog(url) + " did not complete (circuit open or transport error)", + FAIL_LOG_INTERVAL_MS); + return; + } final int response = call.response(); if (!CircuitBreakerUrl.isSuccessResponse(response)) { Logger.warnEvery(this.getClass(), diff --git a/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java b/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java index 80d832eee4c..fde57fb008d 100644 --- a/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java +++ b/dotCMS/src/test/java/com/dotcms/cost/RequestCostPublisherTest.java @@ -70,6 +70,23 @@ public void test_isEnabled_returnsTrue_whenUrlAndTokenAreBothSet() { assertTrue("publisher must activate when both keys are present", publisher.isEnabled()); } + /** + * Regression: a token that is only CRLF / whitespace must NOT activate the publisher. + * If the gate used the raw token, isSet would return true, the publisher would activate, + * and {@code post()}'s sanitizer would strip the token to empty — resulting in an + * unauthenticated POST to the collector. Worse than staying disabled. + */ + @Test + public void test_isEnabled_returnsFalse_whenTokenIsOnlyWhitespaceOrCrlf() { + // Given + Config.setProperty("REQUEST_COST_PUSH_URL", "https://example.com/cost"); + Config.setProperty("REQUEST_COST_PUSH_TOKEN", " \r\n "); + + // When / Then + assertFalse("whitespace/CRLF-only token must not activate the publisher", + publisher.isEnabled()); + } + @Test public void test_publish_whenDisabled_isANoOp() { // Given — both keys absent From 0a45855930fbd6cde4153d29f94eb8cfbb7efbf7 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 19 May 2026 15:48:42 -0400 Subject: [PATCH 05/12] docs(request-cost): note intentional non-atomic counter read in logRequestCost MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third Claude review flagged that the four LongAdder reads (window then lifetime) aren't atomic relative to each other, so Σ(window) can briefly trail lifetime. That's intentional — atomic would need a lock and the data is observational. Document so the next reader doesn't "fix" it. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) --- dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java index f46df0f5520..d530995901a 100644 --- a/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java @@ -83,6 +83,10 @@ private void logRequestCost() { return; } + // The four counter reads below are not atomic relative to each other. Increments + // landing between them are counted in the next window's snapshot but already in + // the lifetime totals — Σ(window) can briefly trail lifetime by a few requests. + // Intentional: observational telemetry, atomic snapshot would need a lock. final long totalRequestsForDuration = this.requestCountForWindow.sumThenReset(); final double totalCostForDuration = this.requestCostForWindow.sumThenReset() / getRequestCostDenominator(); final double costPerRequestForDuration = totalRequestsForDuration == 0 From 8c8c4af3387eb1285134646fbe0eaec3b049b207 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 19 May 2026 15:56:21 -0400 Subject: [PATCH 06/12] fix(request-cost): warn-once on plain-http URL + split fail-log dedup keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth Claude review on PR #35749. Two defense-in-depth items, no behavior contract changes: - Warn-once if REQUEST_COST_PUSH_URL uses http:// (not https://). The bearer token would otherwise traverse the wire in cleartext. Warn-rather-than-refuse so a misconfiguration doesn't silently drop telemetry — the operator sees the log line and fixes it. - Split Logger.warnEvery dedup keys into REQUEST_COST_PUSH_FAIL_TRANSPORT, REQUEST_COST_PUSH_FAIL_HTTP, REQUEST_COST_PUSH_ERR_EXCEPTION. Previously a 500 storm and a circuit trip shared one key, so the second mode was silently suppressed for the rest of the 10-minute window. Tests: 13/13 still passing locally. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../com/dotcms/cost/RequestCostPublisher.java | 31 +++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java index 7885dde333a..6e9270b6485 100644 --- a/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java @@ -9,6 +9,7 @@ import java.net.URI; import java.util.HashMap; import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; import javax.enterprise.context.ApplicationScoped; /** @@ -32,6 +33,7 @@ public class RequestCostPublisher { private static final ObjectMapper MAPPER = new ObjectMapper(); private static final int FAIL_LOG_INTERVAL_MS = 10 * 60 * 1000; + private final AtomicBoolean httpSchemeWarned = new AtomicBoolean(false); public boolean isEnabled() { // Use the sanitized token in the gate so a whitespace-or-CRLF-only token @@ -68,6 +70,7 @@ private void post(final RequestCostSnapshot snapshot) { if (!UtilMethods.isSet(url)) { return; } + warnOncePlainHttp(url); try { final Map headers = new HashMap<>(); headers.put("Content-Type", "application/json"); @@ -88,7 +91,7 @@ private void post(final RequestCostSnapshot snapshot) { call.doString(); if (!call.isProcessed()) { Logger.warnEvery(this.getClass(), - "REQUEST_COST_PUSH_FAIL", + "REQUEST_COST_PUSH_FAIL_TRANSPORT", "Request cost push to " + sanitizeUrlForLog(url) + " did not complete (circuit open or transport error)", FAIL_LOG_INTERVAL_MS); return; @@ -96,18 +99,40 @@ private void post(final RequestCostSnapshot snapshot) { final int response = call.response(); if (!CircuitBreakerUrl.isSuccessResponse(response)) { Logger.warnEvery(this.getClass(), - "REQUEST_COST_PUSH_FAIL", + "REQUEST_COST_PUSH_FAIL_HTTP", "Request cost push to " + sanitizeUrlForLog(url) + " returned HTTP " + response, FAIL_LOG_INTERVAL_MS); } } catch (final Exception e) { Logger.warnEvery(this.getClass(), - "REQUEST_COST_PUSH_ERR", + "REQUEST_COST_PUSH_ERR_EXCEPTION", "Request cost push to " + sanitizeUrlForLog(url) + " failed: " + e.getMessage(), FAIL_LOG_INTERVAL_MS); } } + /** + * Warns once (per process lifetime) if the configured URL uses the plain {@code http://} + * scheme — the bearer token would otherwise traverse the wire in cleartext. Stays a warning + * rather than a refusal so a misconfiguration doesn't silently drop telemetry. + */ + private void warnOncePlainHttp(final String url) { + if (httpSchemeWarned.get()) { + return; + } + try { + final String scheme = URI.create(url).getScheme(); + if (scheme != null && scheme.equalsIgnoreCase("http") + && httpSchemeWarned.compareAndSet(false, true)) { + Logger.warn(this.getClass(), + "REQUEST_COST_PUSH_URL uses plain http:// — bearer token will be sent " + + "in cleartext. Use https:// for any non-loopback destination."); + } + } catch (final Exception ignored) { + // sanitizer is best-effort + } + } + /** * Strips CR/LF and surrounding whitespace from a header value. A misconfigured * {@code REQUEST_COST_PUSH_TOKEN} containing CRLF would otherwise enable HTTP header From 0f2f5bf1e7b32dd69e057521fc00980b0533f005 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 19 May 2026 16:17:13 -0400 Subject: [PATCH 07/12] refactor(request-cost): rename snapshot field environmentId -> serverId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fifth Claude review on PR #35749 (and the user's explicit call) flagged that the wire field environmentId was misleading: it was sourced from ConfigUtils.getServerId(), which is a per-JVM/per-node identifier — every node in a cluster sends a different value. Across Grafana that reads inverted. Rename to serverId so the wire contract matches the source data exactly: - clusterId = cluster - serverId = node Done before any collector consumes the payload, so the wire format is locked in correctly from the first emit. Tests: 13/13 still passing locally. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/main/java/com/dotcms/cost/RequestCostSnapshot.java | 6 +++--- .../test/java/com/dotcms/cost/RequestCostSnapshotTest.java | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostSnapshot.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostSnapshot.java index b294eedd55e..a3c17c19c65 100644 --- a/dotCMS/src/main/java/com/dotcms/cost/RequestCostSnapshot.java +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostSnapshot.java @@ -14,7 +14,7 @@ public final class RequestCostSnapshot { public final String clusterId; - public final String environmentId; + public final String serverId; public final String timestamp; public final int windowSeconds; public final long windowRequests; @@ -26,7 +26,7 @@ public final class RequestCostSnapshot { public RequestCostSnapshot( final String clusterId, - final String environmentId, + final String serverId, final String timestamp, final int windowSeconds, final long windowRequests, @@ -36,7 +36,7 @@ public RequestCostSnapshot( final double lifetimeTokens, final double lifetimeAvgTokensPerRequest) { this.clusterId = clusterId; - this.environmentId = environmentId; + this.serverId = serverId; this.timestamp = timestamp; this.windowSeconds = windowSeconds; this.windowRequests = windowRequests; diff --git a/dotCMS/src/test/java/com/dotcms/cost/RequestCostSnapshotTest.java b/dotCMS/src/test/java/com/dotcms/cost/RequestCostSnapshotTest.java index c7ab37715a1..873e60e6440 100644 --- a/dotCMS/src/test/java/com/dotcms/cost/RequestCostSnapshotTest.java +++ b/dotCMS/src/test/java/com/dotcms/cost/RequestCostSnapshotTest.java @@ -38,7 +38,7 @@ public void test_serialization_includesAllExpectedFields() throws Exception { // Then assertTrue("missing clusterId", json.has("clusterId")); - assertTrue("missing environmentId", json.has("environmentId")); + assertTrue("missing serverId", json.has("serverId")); assertTrue("missing timestamp", json.has("timestamp")); assertTrue("missing windowSeconds", json.has("windowSeconds")); assertTrue("missing windowRequests", json.has("windowRequests")); @@ -56,7 +56,7 @@ public void test_serialization_preservesValues() throws Exception { // Then assertEquals("cluster-1", json.get("clusterId").asText()); - assertEquals("server-7", json.get("environmentId").asText()); + assertEquals("server-7", json.get("serverId").asText()); assertEquals("2026-05-19T18:48:00Z", json.get("timestamp").asText()); assertEquals(60, json.get("windowSeconds").asInt()); assertEquals(1234L, json.get("windowRequests").asLong()); From 66efc573e24fb71b726623d4f5db9e2d1c39ae41 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 19 May 2026 16:24:37 -0400 Subject: [PATCH 08/12] fix(request-cost): clamp denominator, drop redundant Content-Type, re-check token on post MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sixth Claude review on PR #35749. No blocker, but three real defensive fixes: - Clamp REQUEST_COST_DENOMINATOR to >= 1.0 at init. A misconfigured 0 would produce Infinity/NaN in the snapshot, which Jackson serializes as literals invalid under strict JSON parsers — collector-side breakage on the first emit. - Drop the explicit "Content-Type: application/json" header. CircuitBreakerUrl already sniffs rawData starting with '{' and applies the JSON content type itself; setting both can emit a duplicate header on some Apache HttpClient versions. - Re-check the sanitized token in post() as well as the URL. The config can be cleared between snapshot submission and the executor running the POST. Posting without an Authorization header is a worse failure mode than not posting — the collector sees authenticated-looking traffic that isn't. 13/13 tests passing locally. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../java/com/dotcms/cost/RequestCostApiImpl.java | 5 ++++- .../java/com/dotcms/cost/RequestCostPublisher.java | 14 ++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java index d530995901a..7ace1dfc1aa 100644 --- a/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java @@ -62,7 +62,10 @@ public RequestCostApiImpl(Boolean enable) { @PostConstruct public void init() { this.requestCostTimeWindowSeconds = Config.getIntProperty("REQUEST_COST_TIME_WINDOW_SECONDS", 60); - this.requestCostDenominator = Config.getFloatProperty("REQUEST_COST_DENOMINATOR", 1.0f); + // Clamp to >= 1.0 so a misconfigured 0 doesn't produce Infinity/NaN in the snapshot — + // those serialize as JSON-invalid literals and break strict parsers on the collector side. + this.requestCostDenominator = Math.max(1.0d, + Config.getFloatProperty("REQUEST_COST_DENOMINATOR", 1.0f)); this.scheduler = Executors.newSingleThreadScheduledExecutor( r -> { diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java index 6e9270b6485..12d94eed412 100644 --- a/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java @@ -67,17 +67,19 @@ public void publish(final RequestCostSnapshot snapshot) { private void post(final RequestCostSnapshot snapshot) { final String url = getUrl(); - if (!UtilMethods.isSet(url)) { + final String token = sanitizeHeaderValue(getToken()); + // Re-check both pieces — config may have been cleared between submit and execute, and + // posting without an Authorization header is a worse failure mode than not posting at all. + if (!UtilMethods.isSet(url) || !UtilMethods.isSet(token)) { return; } warnOncePlainHttp(url); try { + // CircuitBreakerUrl sniffs the rawData and applies Content-Type automatically when + // the payload starts with '{', so we don't set it explicitly (would risk a duplicate + // header on some Apache HttpClient versions). final Map headers = new HashMap<>(); - headers.put("Content-Type", "application/json"); - final String token = sanitizeHeaderValue(getToken()); - if (UtilMethods.isSet(token)) { - headers.put("Authorization", "Bearer " + token); - } + headers.put("Authorization", "Bearer " + token); final CircuitBreakerUrl call = CircuitBreakerUrl.builder() .setMethod(CircuitBreakerUrl.Method.POST) From 017e76e4cee4d1833a5b106846abcff9b7f933c4 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Tue, 19 May 2026 16:32:02 -0400 Subject: [PATCH 09/12] fix(request-cost): tighten Jackson visibility, coalesce null cluster/server ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seventh Claude review on PR #35749. Two real defensive fixes, then I'm done chasing AI feedback — verdict has been "no blocker" since round 3 and the remaining items are diminishing-returns. - Snapshot Jackson @JsonAutoDetect: ANY -> PUBLIC_ONLY. All wire fields are already public final so behavior is identical, but the explicit lock makes it impossible to accidentally leak a future private field onto the wire. The "exactly 10 fields" test becomes belt-and-suspenders rather than the only line of defense. - nullSafe() coalesces null returns from ClusterFactory.getClusterId() / ConfigUtils.getServerId(). Try.getOrElse only fires on throw — null returns during early startup would otherwise have serialized "clusterId": null on the wire instead of the documented "unknown" fallback. 13/13 tests passing locally. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../java/com/dotcms/cost/RequestCostApiImpl.java | 12 ++++++++++-- .../java/com/dotcms/cost/RequestCostSnapshot.java | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java index 7ace1dfc1aa..5401e5951c8 100644 --- a/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java @@ -8,6 +8,7 @@ import com.dotmarketing.util.Config; import com.dotmarketing.util.ConfigUtils; import com.dotmarketing.util.Logger; +import com.dotmarketing.util.UtilMethods; import com.liferay.portal.model.User; import com.liferay.portal.util.PortalUtil; import io.vavr.control.Try; @@ -80,6 +81,11 @@ public void init() { } private volatile boolean skipZeroRequests = false; + + private static String nullSafe(final String value) { + return UtilMethods.isSet(value) ? value : "unknown"; + } + private void logRequestCost() { try { if (!isAccountingEnabled()) { @@ -124,8 +130,10 @@ private void logRequestCost() { if (publisher.isEnabled()) { publisher.publish(new RequestCostSnapshot( - Try.of(ClusterFactory::getClusterId).getOrElse("unknown"), - Try.of(ConfigUtils::getServerId).getOrElse("unknown"), + // Try.getOrElse only fires on throw — also coalesce null returns since + // these lookups can transiently return null during early startup. + nullSafe(Try.of(ClusterFactory::getClusterId).getOrNull()), + nullSafe(Try.of(ConfigUtils::getServerId).getOrNull()), Instant.now().truncatedTo(ChronoUnit.SECONDS).toString(), requestCostTimeWindowSeconds, totalRequestsForDuration, diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostSnapshot.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostSnapshot.java index a3c17c19c65..deec0addee0 100644 --- a/dotCMS/src/main/java/com/dotcms/cost/RequestCostSnapshot.java +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostSnapshot.java @@ -8,7 +8,7 @@ * endpoint on each scheduled tick. One snapshot = one point in the time series. */ @JsonAutoDetect( - fieldVisibility = Visibility.ANY, + fieldVisibility = Visibility.PUBLIC_ONLY, getterVisibility = Visibility.NONE, isGetterVisibility = Visibility.NONE) public final class RequestCostSnapshot { From aed64773e2c153993b6dff5582c2dbaab08bc9f6 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Wed, 20 May 2026 13:28:23 -0400 Subject: [PATCH 10/12] chore(ai-workflows): add wezell to backend reviewer pilot allowlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So the multi-agent Claude backend reviewer fires on PRs I author and can submit a formal --approve / --request-changes (which the orchestrator's automatic-review path can't do). Touches .github/ which is owned by @dotCMS/dotDevelopers per CODEOWNERS — splitting from the feature commits so it's an obvious one-line audit. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/ai_claude-backend-reviewer.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ai_claude-backend-reviewer.yml b/.github/workflows/ai_claude-backend-reviewer.yml index 0de5e7a5ea9..70cf50c126d 100644 --- a/.github/workflows/ai_claude-backend-reviewer.yml +++ b/.github/workflows/ai_claude-backend-reviewer.yml @@ -23,7 +23,7 @@ jobs: env: AUTHOR: ${{ github.event.pull_request.user.login }} run: | - PILOT_AUTHORS='["dsolistorres","gortiz-dotcms","dsilvam"]' + PILOT_AUTHORS='["dsolistorres","gortiz-dotcms","dsilvam","wezell"]' if echo "$PILOT_AUTHORS" | jq -e --arg a "$AUTHOR" 'index($a) != null' > /dev/null; then echo "authorized=true" >> $GITHUB_OUTPUT echo "✅ $AUTHOR is in the pilot list" From 885ed1571c542cffd2fd4603ef5668d1166ff762 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Wed, 20 May 2026 13:29:31 -0400 Subject: [PATCH 11/12] Revert "chore(ai-workflows): add wezell to backend reviewer pilot allowlist" This reverts commit aed64773e2c153993b6dff5582c2dbaab08bc9f6. --- .github/workflows/ai_claude-backend-reviewer.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ai_claude-backend-reviewer.yml b/.github/workflows/ai_claude-backend-reviewer.yml index 70cf50c126d..0de5e7a5ea9 100644 --- a/.github/workflows/ai_claude-backend-reviewer.yml +++ b/.github/workflows/ai_claude-backend-reviewer.yml @@ -23,7 +23,7 @@ jobs: env: AUTHOR: ${{ github.event.pull_request.user.login }} run: | - PILOT_AUTHORS='["dsolistorres","gortiz-dotcms","dsilvam","wezell"]' + PILOT_AUTHORS='["dsolistorres","gortiz-dotcms","dsilvam"]' if echo "$PILOT_AUTHORS" | jq -e --arg a "$AUTHOR" 'index($a) != null' > /dev/null; then echo "authorized=true" >> $GITHUB_OUTPUT echo "✅ $AUTHOR is in the pilot list" From d6b02a660b2ebb659fffbe696e3c5089e6647edc Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Wed, 20 May 2026 16:00:13 -0400 Subject: [PATCH 12/12] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java index 12d94eed412..0ee3445f096 100644 --- a/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java +++ b/dotCMS/src/main/java/com/dotcms/cost/RequestCostPublisher.java @@ -110,6 +110,9 @@ private void post(final RequestCostSnapshot snapshot) { "REQUEST_COST_PUSH_ERR_EXCEPTION", "Request cost push to " + sanitizeUrlForLog(url) + " failed: " + e.getMessage(), FAIL_LOG_INTERVAL_MS); + Logger.debug(this.getClass(), + "Request cost push to " + sanitizeUrlForLog(url) + " failed with exception", + e); } }