Skip to content

feat(content): observe content body size and emit a metric on mismatch - #1926

Open
LautaroPetaccio wants to merge 1 commit into
mainfrom
fix/observe-content-body-size
Open

feat(content): observe content body size and emit a metric on mismatch#1926
LautaroPetaccio wants to merge 1 commit into
mainfrom
fix/observe-content-body-size

Conversation

@LautaroPetaccio

Copy link
Copy Markdown
Contributor

Summary

  • Adds a dcl_content_short_response_total{reason} counter that fires whenever a /contents/:hashId, entity-image, or entity-thumbnail response streams fewer (or more) bytes than the size declared by storage.
  • Tightens Content-Length header emission in createContentFileHeaders: now uses content.size != null instead of if (content.size), so a legitimate 0-byte file emits Content-Length: 0 instead of being elided into a chunked-encoded empty response.

Why

The content endpoint currently has no signal for a class of failure we cannot observe today: a response that begins normally and ends short of content.size for any reason. Examples we've now seen in practice:

  • Storage backend (S3) returning a successful 200 with a body shorter than the declared ContentLength.
  • MIME-sniffing (FileTypeParser.fromStream) consuming bytes from the first stream and destroy()ing it, with the second asRawStream() call landing in some unexpected state.
  • An upstream proxy (Cloudflare, CloudFront, etc.) mangling chunked transfer encoding and forwarding an empty stream to clients.

At the HTTP layer all of these look identical to a successful response — 200 OK, headers fine, stream cleanly closed at zero bytes — so an aggressive cache configuration in front of the catalyst (e.g. Page Rule: Cache Everything, Edge Cache TTL: 1 year) will happily latch onto the broken body and serve it for the full TTL. We've recently observed exactly this incident in production on peer.decentraland.today, where Cloudflare's IAD POP cached 0-byte 200 responses for a scene's glb assets and pinned them for the configured year, breaking asset-bundle conversion for ~24 hours.

The catalyst code itself isn't the root cause — but it's the only layer that knows both the expected size (from storage) and the actual size (the bytes flowing through the response) at the same time. Adding the comparison here turns "we have no idea this happened until external clients report broken scenes" into "we get a step-change on a counter ops can alert on."

How

observeContentBodySize(source, expectedSize, hash, components) in src/controllers/utils.ts

Wraps the body Readable in a stream.Transform that:

  1. Passes chunks through unmodified (respecting backpressure — using a Transform instead of attaching a 'data' listener directly avoids racing the HTTP framework's consumer).
  2. Counts bytes as they go.
  3. In the flush callback (fired after the source emits 'end'), compares the count to expectedSize. On mismatch, increments dcl_content_short_response_total{reason: 'truncated'} and emits a warn-level log including hash, expectedSize, and observed.
  4. On a source error, forwards the error to the wrapped stream (so the HTTP framework still tears down cleanly) and increments the counter with reason: 'error'.

When expectedSize is null (uncommon — only happens for some range responses where the clamped end isn't known), the helper returns the source unchanged. The metric tracks only cases where storage actually knows what size the body should be.

Wired into three handlers

getContentHandler, getEntityImageHandler, and getEntityThumbnailHandler all follow the same pattern: a HEAD/GET handler that returns body: await content.asRawStream() on GET. Each now wraps that stream with observeContentBodySize and picks 'metrics' | 'logs' from AppComponents to do it.

Content-Length: 0 fix

The previous if (content.size) check elided the header for both null/undefined (legitimately unknown) and 0 (legitimately empty). The latter is a bug: a 0-byte stored object should advertise Content-Length: 0 so clients and intermediaries can distinguish "intentionally empty body" from "chunked encoding, body might be anything." The fix swaps the check for content.size != null, preserving the existing behaviour for the unknown-size case while emitting the explicit 0 for empty files.

Why this isn't redundant with origin-side instrumentation

If the catalyst is behind a proxy (Cloudflare → ALB → catalyst), an HTTP-level access log on the proxy can also detect truncation by comparing bytes_sent against the response's Content-Length. That's useful too, but it requires:

  • Both the response and the access log to be reachable to an operator with the right permissions.
  • The proxy's logging to be configured for bytes_sent (not always default).
  • The operator to know which URLs to grep, in advance.

The metric we add here:

  • Fires on the catalyst itself, in the same process that's reading from storage. No cross-system join.
  • Is automatically aggregated by Prometheus alongside every other catalyst metric.
  • Doesn't require knowing which URL is affected — it just counts mismatches across the entire content endpoint.

The two layers are complementary, not redundant.

Tests

Unit-tested the helper in isolation in test/unit/controllers/utils.spec.ts with five new cases under a describe('when observing content body size', …) block:

  • Returns the source unchanged when expectedSize is null and emits no metric.
  • Matching size: no metric, no warn log.
  • Body shorter than expected: emits reason: 'truncated' and one warn log with the right payload.
  • Body longer than expected: same metric (the counter tracks "size mismatch" — either direction is a signal).
  • Source error mid-transfer: emits reason: 'error', propagates the source error to the wrapped stream so the framework can tear down.

Also updated test/unit/controllers/conditional-request.spec.ts to pass minimal metrics and logs mocks alongside storage so the existing 200-path tests (which now reach the body observer) don't crash on components.logs.getLogger. The 304-path tests were unaffected.

  • yarn build — clean (tsc -b)
  • yarn jest test/unit — 231 passed, 26 test suites
  • lint-staged ran on commit (eslint --fix) — clean

Out of scope (worth follow-ups)

  • Integration test that simulates a truncated origin pull end-to-end. Would need either a fake storage component that returns a short stream against a known content.size, or a stubbed S3 backend. The unit test already exercises every branch of the observer in isolation, so the integration path is the controller wiring — which TypeScript already validates at compile time.
  • A second metric for Content-Length: 0 responses to known-non-zero hashes. Tighter signal, but the existing counter already fires on these.
  • Migration of getContentHandler and friends to the WKC logic/ layer so the body-observe wiring lives in a logic component rather than the controller. The current change matches the existing controller pattern; reshuffling layers should be its own PR.

Wraps the response body stream in `getContentHandler`,
`getEntityImageHandler`, and `getEntityThumbnailHandler` with a
passthrough that compares the bytes streamed to the size declared by
storage; emits the new `dcl_content_short_response_total` counter
(labels: `reason=truncated|error`) and a warning log when the two
disagree.

The motivation is the failure mode we currently have no signal for: a
content response that begins normally and ends short of `content.size`
for any reason (storage backend hiccup, MIME-sniffing leaving the
stream in a bad state, an upstream proxy mangling chunked encoding).
At the HTTP layer a truncated 200 OK is indistinguishable from a
successful response, so aggressively-cached CDNs in front of the
catalyst can latch onto the broken body for the lifetime of their TTL
without us ever seeing it on our side. With this metric, the same
incident shows up as a step-change on a counter that ops can alert on.

The observer is opt-out: when `expectedSize` is null (uncommon — only
range responses where the clamped end isn't known) it returns the
source stream unchanged. Source-stream errors are forwarded to the
wrapped stream so the HTTP framework still tears the response down
cleanly; the `error` label lets ops distinguish stream-errored short
responses from cleanly-ended short responses (the latter is the more
alarming case — it means the origin claimed to send N bytes and then
ended at <N without raising an error).

Also tightens the `Content-Length` header construction in
`createContentFileHeaders`: switch from `if (content.size)` to
`if (content.size != null)` so a legitimate 0-byte file emits
`Content-Length: 0` instead of being elided. A missing
`Content-Length` forces chunked transfer encoding, which is
indistinguishable from an empty-but-cleanly-terminated chunk stream at
upstream caches.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant