Skip to content

fix: return manifest unknown when content exceeds the read limit - #61

Merged
AKatruwar merged 1 commit into
digitalocean:masterfrom
harshmaru7:hmaru/manifest-read-limit-404
Aug 20, 2026
Merged

fix: return manifest unknown when content exceeds the read limit#61
AKatruwar merged 1 commit into
digitalocean:masterfrom
harshmaru7:hmaru/manifest-read-limit-404

Conversation

@harshmaru7

@harshmaru7 harshmaru7 commented Aug 20, 2026

Copy link
Copy Markdown

Summary

  • Fetching a blob digest (layer/config) through the manifest service reads the raw blob content; for blobs over 4 MiB (maxBlobGetSize) this fails with the untyped error storage: read exceeds limit, which the registry API maps to HTTP 500 — burning DOCR's error-rate SLO when a client hits the wrong endpoint.
  • Manifest PUTs are capped at the same 4 MiB (maxManifestBodySize in the handlers), so oversized content reached through the manifest endpoint can never be a valid manifest. This maps the read-limit error to distribution.ErrManifestUnknownRevision, which the API layer already translates to 404 MANIFEST_UNKNOWN.

Changes

  • registry/storage/io.go: promote the inline storage: read exceeds limit error to a package sentinel errReadExceedsLimit (same message, now matchable).
  • registry/storage/manifeststore.go: in manifestStore.Get, map errReadExceedsLimit to ErrManifestUnknownRevision alongside the existing ErrBlobUnknown case. No happy-path changes.
  • registry/storage/manifeststore_test.go: add TestManifestGetOversizedBlobContent following the DOCR-2302 (feat: enhance manifest retrieval error handling #60) test pattern.

Context

Production incident (docr-alerts, Aug 19–20): a customer tool fetched blob digests via GET /v2/<repo>/manifests/<digest> for blobs >4 MiB, producing a sustained stream of 500s and retries for ~10.5h. The same digests succeed via the blobs endpoint. This is the >4 MiB counterpart of the sub-4 MiB non-manifest-content cases fixed in #60 (DOCR-2302).

Once merged, docr will pick this up via a go.mod pseudo-version bump + go mod vendor (replacing https://github.internal.digitalocean.com/digitalocean/docr/pull/310, which had edited vendor files directly).

Testing

  • go test ./registry/storage/ -run TestManifestGet -v — all pass, including the new test.
  • The four failures in the full package run (TestSimpleBlobUpload, TestBlobMount, TestLinkedBlobStoreCreateWithMountFrom, TestManifestStorage) are pre-existing on master (verified via stash) and unrelated.

Fetching a blob digest (layer/config) through the manifest service reads
the raw blob; for blobs over 4 MiB this failed with the untyped error
"storage: read exceeds limit", which the registry API maps to HTTP 500.
Manifest puts are capped at the same 4 MiB, so oversized content can
never be a valid manifest. Promote the error to a package sentinel and
map it to ErrManifestUnknownRevision (404) in manifestStore.Get.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copilot AI lite review requested due to automatic review settings August 20, 2026 10:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves registry error mapping when a client incorrectly requests a large blob (layer/config) digest via the manifests endpoint. When the blob content exceeds the internal read limit (4 MiB), the storage layer previously returned an untyped error that surfaced as HTTP 500; this change maps that scenario to ErrManifestUnknownRevision, which the API layer translates to 404 MANIFEST_UNKNOWN.

Changes:

  • Introduces a matchable sentinel error for the “storage: read exceeds limit” condition.
  • Updates manifestStore.Get to treat “read exceeds limit” the same as “blob unknown” for manifest retrieval (return ErrManifestUnknownRevision).
  • Adds a regression test covering oversized blob content fetched through the manifest service.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
registry/storage/io.go Promotes the read-limit error to a package sentinel (errReadExceedsLimit) for reliable identification.
registry/storage/manifeststore.go Maps the read-limit sentinel to ErrManifestUnknownRevision so the handler returns 404 instead of 500.
registry/storage/manifeststore_test.go Adds TestManifestGetOversizedBlobContent to ensure oversized blob digests fetched as manifests return ErrManifestUnknownRevision.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@Bala-Nallamilli

Copy link
Copy Markdown
Member

Looks good to me — this is the right place for the fix (vs editing vendor in docr).

Checked the main bits:

  • The sentinel + mapping in manifestStore.Get is solid — oversized content through the manifests endpoint should come back as unknown (404) instead of a 500.
  • The new test actually covers the bug. I temporarily removed the mapping and the test failed with storage: read exceeds limit, so we’re good there.

Two small non-blocking notes:

  1. The other “unknown manifest” paths log a warning; this one doesn’t. A quick Warnf here would make these cases easier to spot in prod logs.
  2. Exists can still return true for an oversized blob while Get returns unknown — same shape as before, just calling it out. Fine as a follow-up.

Once this merges, docr can bump the replace pin + re-vendor and we can drop the direct vendor edits in https://github.internal.digitalocean.com/digitalocean/docr/pull/310.

@Bala-Nallamilli Bala-Nallamilli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AKatruwar
AKatruwar merged commit d7252f0 into digitalocean:master Aug 20, 2026
1 check passed
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.

4 participants