[OPIK-7333] [BE] server-side ingestion size guards (request 413 + document 4xx)#7483
Open
YarivHashaiComet wants to merge 18 commits into
Open
[OPIK-7333] [BE] server-side ingestion size guards (request 413 + document 4xx)#7483YarivHashaiComet wants to merge 18 commits into
YarivHashaiComet wants to merge 18 commits into
Conversation
…ment 4xx (OPIK-7333, OPIK-7334) Server-side half of the unbounded-payload OOM fix (parent OPIK-7312; client-side per-span cap is OPIK-7335 / PR #7469). OPIK-7333 - RequestSizeLimitFilter: reject a request whose Content-Length exceeds maxRequestSizeBytes with 413 before the body is read. Guicey auto-installs it as a JAX-RS provider and injects JacksonConfig via the Guicey @config sub-config binding (no explicit module bind) - same style as TraceThreadConfig / OnlineScoringConfig injection. No Content-Length (chunked) falls through to maxDocumentLength. Header parsed as long so >2GB can't slip past an int overflow; malformed/negative -> 400. OPIK-7334 - maxDocumentLength: cap the whole decompressed document on both the HTTP and JsonUtils ObjectMappers via StreamReadConstraints (set directly alongside maxStringLength; the builder normalizes <=0 to unlimited), so an oversized batch aborts mid-parse with 4xx (via JsonProcessingExceptionMapper) before a multi-GB node tree is built. Limits (config-driven, env-overridable): - maxStringLength = 100MB (per-field cap; unchanged; the documented "100MB per field") - maxDocumentLength = 512MB self-hosted default; Comet cloud overrides to 256MB via JACKSON_MAX_DOCUMENT_LENGTH (deployment-side, OPIK-7353) - maxRequestSizeBytes = 512MB self-hosted default; cloud overrides to 256MB via JACKSON_MAX_REQUEST_SIZE_BYTES. Kept == maxDocumentLength so the request filter never rejects a request the document guard would accept. config-test.yml uses lower, payload-calibrated test caps (independent of shipped defaults) so the guard tests trip at modest sizes; JacksonConfigTest verifies the real 512MB defaults. NOTE (deployment): the cloud 256MB override lives in comet-helm/values-production-opik.yaml (component.backend.env), NOT in this repo - tracked as OPIK-7353. Without it, cloud inherits 512MB. Tests (all green via mvn): - Unit (11): RequestSizeLimitFilterTest (over->413, at-limit pass, no-CL pass, >2GB->413, malformed/negative->400); JsonUtilsDocumentLengthTest (over-doc->StreamConstraintsException via streamed InputStream, under parses, unlimited parses, maxStringLength still enforced); JacksonConfigTest (512MB defaults + request==document invariant). - Integration (SpansResourceTest.LargePayloadTests, 4): ~93MB inline succeeds + strips; 253MB single string -> 400 (maxStringLength); 280MB total document -> 400 (maxDocumentLength); buffered client (Grizzly + Expect:100-continue) with Content-Length over cap -> 413. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
java
Contributor
⏱️ pre-commit per-hook timing
⏭️ 31 skipped (no matching files changed)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
config-test.yml added maxDocumentLength/maxRequestSizeBytes without the per-key Default:/Description: docblocks that config.yml carries. Mirror them verbatim so the test config stays structurally in sync with config.yml, matching the sibling maxStringLength entry.
Contributor
Contributor
Emit ingestion_size_guard_rejections_total{guard,endpoint,workspace_id,
workspace_name} so the OPIK-7333/7334 size guards are observable in Grafana.
Neither rejection is otherwise captured: the request-size 413 aborts with a
Response (no throwable) and the document/string-length 4xx is handled by the
more-specific JsonProcessingExceptionMapper, so opik.errors.count never sees
them; the dedicated guard label also separates an oversized-payload reject from
an ordinary malformed-JSON 400.
- IngestionSizeGuardMetrics: @singleton OTel counter, reusing ErrorMetricsResolver
for endpoint + workspace (workspace_id falls back to unknown pre-auth on the 413
path; workspace_name falls back to workspace_id per metrics-instrumentation §2.3).
- RequestSizeLimitFilter: emit guard=request_size on the 413 branch only (malformed/
negative Content-Length 400s are bad framing, not size rejections, and stay uncounted).
- JsonProcessingExceptionMapper: emit guard=document_length/string_length when the
parse failure is a StreamConstraintsException, classified from its message.
Tests: RequestSizeLimitFilterTest asserts emit-on-reject / never-on-pass;
IngestionSizeGuardMetricsTest covers the message classifier. mvn -o compile clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ream-constraint setup (OPIK-7333) Addresses two Baz review findings on #7483: - Cross-field validation (Logical Bug): JacksonConfig only had per-field @min checks, so maxDocumentLength could be set below maxStringLength and reject a payload that already passed the string cap. Add an @AssertTrue invariant (documentLength >= stringLength, or <= 0 for unlimited) so a misordered env override fails fast at startup with a clear message. The config is @Valid, so it's enforced on load. Covered by a new JacksonConfigTest case. - Dedup: the StreamReadConstraints (maxStringLength + maxDocumentLength) build was assembled independently in both OpikApplication (HTTP mapper) and JsonUtils.createConfiguredMapper (internal mapper). Extract a single JsonUtils.applyStreamReadConstraints(mapper, ...) helper used by both, so the two mappers can't drift. Verified: spotless clean, module compiles, JacksonConfigTest + JsonUtilsDocumentLengthTest green (Java 25). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s (OPIK-7333) - Config (A): add an @AssertTrue enforcing maxRequestSizeBytes >= maxDocumentLength (unless the document cap is <= 0 / unlimited), so a request-size cap set below the document cap — which would 413 requests the parser would accept — fails fast at startup. Mirrors the maxDocumentLength >= maxStringLength invariant. - Metrics (C): add a stable `component` dimension (request_filter | json_parser) to ingestion_size_guard_rejections_total, so the 413-filter and 4xx-parser call sites are distinguishable without overloading `guard` (metrics-instrumentation skill §1.4). - Logging (D+E): split JsonProcessingExceptionMapper — expected size-guard StreamConstraintsExceptions log at DEBUG (they're already counted, and were flooding INFO on every oversized ingest); genuine malformed JSON logs at INFO with the exception (stack trace/type) for diagnosis. - Test (B): RequestSizeLimitFilterTest asserts the exact UriInfo passed to recordRequestSizeRejection instead of any(), catching a mislabeled endpoint. Verified (Java 25): spotless clean, module compiles, 17/17 targeted tests pass (JacksonConfigTest, RequestSizeLimitFilterTest, IngestionSizeGuardMetricsTest, JsonUtilsDocumentLengthTest). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Backend Tests - Integration Group 2282 tests 282 ✅ 13m 41s ⏱️ Results for commit 847b160. ♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 3389 tests 389 ✅ 10m 11s ⏱️ Results for commit 847b160. ♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 10309 tests 305 ✅ 8m 9s ⏱️ Results for commit 34394a4. ♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 1426 tests 426 ✅ 8m 24s ⏱️ Results for commit 847b160. ♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Unit Tests2 780 tests 2 778 ✅ 1m 30s ⏱️ Results for commit 847b160. ♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 14 20 files 20 suites 6m 36s ⏱️ Results for commit 847b160. ♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 41 798 tests 1 798 ✅ 9m 25s ⏱️ Results for commit 847b160. ♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 71 591 tests 1 591 ✅ 7m 2s ⏱️ Results for commit 34394a4. ♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 16238 tests 238 ✅ 3m 50s ⏱️ Results for commit 847b160. ♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 15285 tests 285 ✅ 4m 57s ⏱️ Results for commit 847b160. ♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 6175 tests 172 ✅ 3m 21s ⏱️ Results for commit 34394a4. ♻️ This comment has been updated with latest results. |
Contributor
Backend Tests - Integration Group 8379 tests 379 ✅ 4m 56s ⏱️ Results for commit 34394a4. ♻️ This comment has been updated with latest results. |
…size guards (OPIK-7333) Verified locally: unit 19/19, SpansResourceTest LargePayloadTests 4/4 (integration), plus a live dev-runner run driven by the old (pre-truncation) SDK - 10 rejections, all 413, with the guard metric correctly split document_length/string_length by endpoint. - JacksonConfig: maxDocumentLength drops @min (a non-positive value is a deliberate "unlimited" escape hatch that @min turned into a boot failure); its @AssertTrue now carries the floor (>= maxStringLength) AND a 2GB ceiling. maxRequestSizeBytes gains @max(2GB). Beyond ~2GB the guard cannot protect the heap (Java 2^31 String/array limit), so an out-of-range override now fails fast at startup instead of silently disabling the guard. - JsonUtils / JacksonConfig javadoc: clarify the document cap is enforced on stream-based reads (the HTTP boundary), not fully in-memory String/byte[] reads. Read-back hardening is tracked separately as OPIK-7426. - JsonProcessingExceptionMapper: include the workspace id in the genuine-malformed-JSON log. - RequestSizeLimitFilter javadoc: it is a global filter capping every request with a Content-Length (intentional memory protection), not ingestion-only. - IngestionSizeGuardMetrics: document that classifyStreamConstraint depends on Jackson message substrings, now guarded by real-parse tests. - Tests: JsonUtilsDocumentLengthTest adds a typed-bean (SpanBatch) case reproducing the real wrapped-JsonMappingException path (the metric-under-count blind spot); IngestionSizeGuardMetricsTest adds real-Jackson classification tests; JacksonConfigTest covers the 2GB ceiling + @max; SpansResourceTest asserts the 413 body via JsonNode and documents the chunked-transfer dependency; config-test.yml comments corrected (no stale cloud values). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…adoc (OPIK-7333) Second review round: - Response leak: JsonProcessingExceptionMapper returned "Unable to process JSON. " + exception.getMessage(), leaking Jackson internals and fragments of the client's payload into the HTTP error body. It now returns a stable generic message (413 "Request payload exceeds the maximum allowed size."; 400 "Unable to process the request body: it is not valid JSON.") while the full exception (with workspace context) stays in the server log. Added a leak-guard test. - JsonUtils javadoc no longer overstates internal coverage: it states maxDocumentLength is enforced only on stream/reader reads (parser buffer refill), not fully in-memory String/byte[] reads (dropped the misleading "already bounded at ingestion" claim). - Trimmed the verbose javadoc/comments from the previous round to minimal across JacksonConfig, RequestSizeLimitFilter, IngestionSizeGuardMetrics, JsonUtils and the mapper. - Removed the deployment-configurable size numbers (50MB/256MB request/document, the 512MB/cloud narrative) from code comments/javadoc, since they are set by config and drift; only the // 512MB decode of the default literal remains. Customer-facing docs keep the numbers. Verified (Java 25, Docker): unit 19/19; SpansResourceTest LargePayloadTests 4/4 (the 413 tests now assert the generic message end-to-end). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
Collaborator
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
…ses (OPIK-7333) findStreamConstraint only broke on a DIRECT self-cause (cause == cause.getCause()), so a multi-node cause cycle (A -> B -> A, which Throwable.initCause permits) spun the walk forever and pinned the JAX-RS request thread. Replace the fragile self-cause check with a hard depth cap (MAX_CAUSE_DEPTH = 50, far beyond any real chain; no per-call allocation). Adds a timeout-guarded regression test that reproduces the hang on the old code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
YarivHashaiComet
added
test-environment
Contributor
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
Collaborator
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
…on errors (OPIK-7333) The size-guard leak-fix genericized the WHOLE 400 branch of the global JsonProcessingExceptionMapper, changing the error body for every malformed/invalid-JSON request API-wide and breaking ~8 integration tests (spans, project metrics, workspaces, filters, provider keys) that assert the specific 'Unable to process JSON. <detail>' message. Scope the generic message to the 413 size-guard branch (where internal limits / Jackson internals are actually exposed) and restore the informative message on the 400 branch (the caller's own payload + our field names). Status codes unchanged; mapper unit test helper updated to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…333) Reduce the javadoc/comments we added for the ingestion size guards to the load-bearing rationale only (2GB ceiling, no cross-field check, in-memory bypass, Jackson-message fragility, 413-vs-400, do-not-genericize-400, cause-cycle depth cap). Drops restatement, internal skill-doc citations, and redundant @param/@return. Pre-existing comments untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thiagohora
reviewed
Jul 22, 2026
thiagohora
left a comment
thiagohora
left a comment
Contributor
There was a problem hiding this comment.
Opik reviewer (mined from your team's review history)
9 findings — 0 high · 3 medium · 6 low. Suppressed by team conventions: see suppressed.md.
React 👍/👎 on each comment — your feedback helps tune what it flags.
… (OPIK-7333) - Force application/json on the error response: a non-JSON Accept (e.g. OTel protobuf) had no ErrorMessage writer, so the 413/400 was surfacing as a 500 (T2, real bug). - Replace the custom cause-chain walk + MAX_CAUSE_DEPTH with the cycle-safe ExceptionUtils.getThrowableList (T3, code reduction). - Classify the stream constraint: size (document/string length) -> 413; a structural limit (nesting/number depth) -> 400 instead of a misleading 413 (T4). - MAX_CONFIGURABLE_BYTES -> Integer.MAX_VALUE; the old 2^31 was one byte past the real String/array limit (T5). - @priority on RequestSizeLimitFilter so the size guard runs deterministically before auth (413, not 401) (T6). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…PIK-7333) The deserialization-error log logged the full throwable, whose message carries the caller's own body content (potential PII) - a log leak on ingestion endpoints (Baz medium). Log a redacted summary instead: quoted workspace + exception type, no throwable. Also quote the workspace placeholder per opik-backend/SKILL.md. Applies to both 400 log sites; the 413 line stays at debug. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…unclassified (OPIK-7333) The fallback bucket was labelled 'stream_constraint', which reads like a confirmed classification even though it is only ever the catch-all for an unrecognized StreamConstraintsException - misleading dashboards/alerts (Baz low). Rename the constant + emitted value to 'unclassified'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ific overrides (OPIK-7333) Align maxDocumentLength/maxRequestSizeBytes comments with the file's Default/Description house style and remove the self-hosted-vs-cloud (256MB) override prose - the override lives in the deployment env, not this config's comments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thiagohora
previously approved these changes
Jul 22, 2026
…g; config.yml is the source (OPIK-7333) maxDocumentLength/maxRequestSizeBytes no longer hard-code 512MB - the value is supplied by config.yml (Thiago T1: avoid two competing sources of truth). Drops the JacksonConfigTest assertions that checked those in-code defaults; maxStringLength (a symbolic default, not a duplicated literal) is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thiagohora
approved these changes
Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
Server-side half of the unbounded-payload OOM fix (parent OPIK-7312; the client-side per-span cap is OPIK-7335 / #7469).
OPIK-7333 —
RequestSizeLimitFilter(413): rejects a request whoseContent-LengthexceedsmaxRequestSizeByteswith 413 before the body is read, so the JSON parser never starts on an oversized batch. A request with noContent-Length(chunked) falls through and is bounded bymaxDocumentLength. The header is parsed as alongso a >2 GB body can't slip past an int overflow; a malformed/negative header fails closed with 400. Guicey auto-installs it as a JAX-RS provider and injects@Config JacksonConfig(same sub-config pattern asTraceThreadConfig/OnlineScoringConfig).OPIK-7334 —
maxDocumentLength(4xx): caps the whole decompressed document on both the HTTP andJsonUtilsObjectMappers viaStreamReadConstraints, so an oversized batch aborts mid-parse with a 4xx (viaJsonProcessingExceptionMapper) before a multi-GBJsonNodetree is materialized.Limits (config-driven, env-overridable)
maxStringLengthmaxDocumentLengthmaxRequestSizeBytesContent-Length→ 413Request cap is kept
== maxDocumentLengthso the filter never rejects a request the document guard would otherwise accept. Self-hosted gets 512 MB headroom; cloud overrides both to 256 MB viaJACKSON_MAX_DOCUMENT_LENGTH/JACKSON_MAX_REQUEST_SIZE_BYTES.Change checklist
Issues
Testing
Run via
mvn test(JDK 25 + testcontainers), all green:RequestSizeLimitFilterTest(over→413, at-limit pass, no-Content-Lengthpass, >2 GB→413, malformed/negative→400);JsonUtilsDocumentLengthTest(over-doc→StreamConstraintsExceptionvia a streamedInputStream, under parses, unlimited parses,maxStringLengthstill enforced);JacksonConfigTest(512 MB defaults +request == documentinvariant).SpansResourceTest.LargePayloadTests(4): ~93 MB inline base64 → succeeds + stripped to S3; 253 MB single string → 400 (maxStringLength); 280 MB total document → 400 (maxDocumentLength); buffered client (Grizzly +Expect: 100-continue) withContent-Lengthover the cap → 413. (The default chunked test client bypasses the request filter, which is why the 413 test uses a buffered client.)config-test.ymluses lower, payload-calibrated caps so the guard tests trip at modest sizes;JacksonConfigTestverifies the real 512 MB shipped defaults.Documentation
config.yml.comet-helm/values-production-opik.yaml(component.backend.env), tracked as OPIK-7353. Without it, cloud inherits the 512 MB self-hosted default.🤖 Generated with Claude Code