Both OCI remote read arms answer a cancelled cache lookup with a 500 and an ERROR

What

Both OCI remote read arms look the cache up before they decide anything, and neither checks whether the client is still there when that lookup fails:

  • ServeRemoteRead's blob arm in internal/format/oci/remote_blob_serve.go, answering oci remote blob read: cache lookup failed.
  • RemoteManifestHandler.ServeRemoteRead's equivalent in internal/format/oci/remote_serve.go, answering oci remote manifest read: cache lookup failed.

Both are greppable by those two message strings, which is how to find them; the files move under them often enough that line numbers here would be wrong within the week.

Both go through remoteReadSubject.writeInternal, which writes a 500 INTERNAL and logs at ERROR. The lookup runs on the request context, so a client that hangs up while it is in flight fails the query with its own cancellation and is answered as this service's fault.

Why it matters

Unlike the fill paths, these two sites are reachable on main today: a remote repository serves cached reads whether or not a fill seam is wired, so nothing gates them behind an option. Container clients abort pulls routinely, and the lookup is a database round trip, which is one of the places a request actually blocks.

The cost is the usual pair: client hangups charged to this service's 5xx rate, and one ERROR line per abandoned pull in a log an operator filters on for real faults.

The guard already exists

remoteReadSubject.writeIfClientClosed stamps 499, writes no envelope, and reports whether it fired. It keys on the request context rather than on a wrapped sentinel, and its doc says why. The manifest arm's post-commit lookup is the shape to copy, in RemoteManifestHandler.fillCacheMiss's post-commit lookup in internal/format/oci/remote_manifest.go:

row, err := read.cache.LookupRow(ctx, read.subject.path)
if err != nil {
	if read.subject.writeIfClientClosed(ctx, w) {
		return
	}
	...

Checked after the call rather than before it, because the window that matters is a client leaving during the round trip; an entry check only catches one already gone.

Fix shape, and the trap in the cheaper version

Two options, and the second needs an argument this issue does not settle:

  1. Per-site, on each error branch, as the manifest arm's post-commit lookup already does. Two sites, no shared-behaviour question.
  2. Inside writeInternal, which would cover every current and future caller at once. This is the shape #744 proposes for logAndWriteInternalError on the maven and npm delete arms, and the argument for it is the same: one change covers every site.

What option 2 has to establish first is that the ctx every writeInternal caller holds is the request's context. That is not free here: internal/remote's fill detaches work with context.WithoutCancel, and writeIfClientClosed's own doc records that an inherited timeout or a pool deadline can wrap context.Canceled with the client still connected, which is a server fault owed a logged 500. A blanket check in the shared writer turns every such case into a silent 499 with no line saying what failed.

Scope

The remote fill paths are not this issue. The blob fill's own windows are closed by Step 15 part 2 of the S16 container remote work (!1903 (merged)), which guards its entry, its surface build, its mapping, and its read-back; the manifest fill guards its three. What is left after those land is the pair of entry lookups above, which sit ahead of any fill and answer a cached read.

Related

  • #744 — the same defect class on the maven and npm delete arms, through a different shared helper. Whichever lands first should say which fix shape it chose and why, because the two decisions are the same decision.
Edited by Sylvia Shen