feat(content): observe content body size and emit a metric on mismatch - #1926
Open
LautaroPetaccio wants to merge 1 commit into
Open
feat(content): observe content body size and emit a metric on mismatch#1926LautaroPetaccio wants to merge 1 commit into
LautaroPetaccio wants to merge 1 commit into
Conversation
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.
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
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.
Summary
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.Content-Lengthheader emission increateContentFileHeaders: now usescontent.size != nullinstead ofif (content.size), so a legitimate 0-byte file emitsContent-Length: 0instead 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.sizefor any reason. Examples we've now seen in practice:ContentLength.FileTypeParser.fromStream) consuming bytes from the first stream anddestroy()ing it, with the secondasRawStream()call landing in some unexpected state.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 onpeer.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)insrc/controllers/utils.tsWraps the body
Readablein astream.Transformthat:'data'listener directly avoids racing the HTTP framework's consumer).flushcallback (fired after the source emits'end'), compares the count toexpectedSize. On mismatch, incrementsdcl_content_short_response_total{reason: 'truncated'}and emits awarn-level log includinghash,expectedSize, andobserved.reason: 'error'.When
expectedSizeisnull(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, andgetEntityThumbnailHandlerall follow the same pattern: a HEAD/GET handler that returnsbody: await content.asRawStream()on GET. Each now wraps that stream withobserveContentBodySizeand picks'metrics' | 'logs'fromAppComponentsto do it.Content-Length: 0fixThe previous
if (content.size)check elided the header for bothnull/undefined(legitimately unknown) and0(legitimately empty). The latter is a bug: a 0-byte stored object should advertiseContent-Length: 0so clients and intermediaries can distinguish "intentionally empty body" from "chunked encoding, body might be anything." The fix swaps the check forcontent.size != null, preserving the existing behaviour for the unknown-size case while emitting the explicit0for 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_sentagainst the response'sContent-Length. That's useful too, but it requires:bytes_sent(not always default).The metric we add here:
The two layers are complementary, not redundant.
Tests
Unit-tested the helper in isolation in
test/unit/controllers/utils.spec.tswith five new cases under adescribe('when observing content body size', …)block:expectedSizeis null and emits no metric.reason: 'truncated'and one warn log with the right payload.reason: 'error', propagates the source error to the wrapped stream so the framework can tear down.Also updated
test/unit/controllers/conditional-request.spec.tsto pass minimalmetricsandlogsmocks alongsidestorageso the existing 200-path tests (which now reach the body observer) don't crash oncomponents.logs.getLogger. The 304-path tests were unaffected.yarn build— clean (tsc -b)yarn jest test/unit— 231 passed, 26 test suiteslint-stagedran on commit (eslint --fix) — cleanOut of scope (worth follow-ups)
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.Content-Length: 0responses to known-non-zero hashes. Tighter signal, but the existing counter already fires on these.getContentHandlerand friends to the WKClogic/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.