Repository navigation
fix(blob/http): mark a blob-store miss with a header so Exists tells it from a proxy 404 - #2057
Open
codenlighten wants to merge 2 commits into
Open
codenlighten wants to merge 2 commits into
codenlighten wants to merge 2 commits into
Conversation
…it from a proxy 404 Fixes bsv-blockchain#1965. HTTPStore.Exists sends HEAD, and a HEAD response has no body, so it could not use the NotFoundMsg body-check that bsv-blockchain#1963 gave Get: it read any 404 as a miss. A wrong base URL or a proxy answering 404 therefore made every blob look absent to callers that branch on Exists. The blob server now marks its own miss with X-Blob-Miss: 1 on every path that answers one -- handleExists (HEAD) and writeStoreError (GET, ranged GET, PATCH). Exists reads a 404 as a miss only when the header is present; any other 404 is logged (the address in the log line, not the error text, as bsv-blockchain#1963 does) and, when the store URL carries ?requireMissHeader=true, returned as a StorageError. The flag is off by default so a mixed fleet still works with servers that predate the header; it can become the default once every server sends it. Get prefers the header too, falling back to the NotFoundMsg body, and drains the small 404 body first so the connection stays reusable. The MissHeader constant lives in the client package and the server references it as it already does for NotFoundMsg, so the two cannot drift. Tests: Exists returns not-held for a 404 with the header and for a bare 404 by default, a StorageError for a bare 404 under requireMissHeader, and still not-held for a header-carrying 404 under requireMissHeader; the server marks both its HEAD and Get misses. Reverting the behavioural lines fails these, so they pin the behaviour and do not vacuously pass.
icellan
requested changes
Oct 26, 2026
icellan
left a comment
icellan
left a comment
Contributor
There was a problem hiding this comment.
Header approach is right. Two things block it: the strict mode that fixes the issue is off by default, and the GET header branch has no test.
oskarszoon
approved these changes
Oct 26, 2026
oskarszoon
left a comment
oskarszoon
left a comment
Contributor
There was a problem hiding this comment.
The miss header is applied on the server paths and the client distinguishes marked misses from foreign 404s when requireMissHeader=true. I found no P0/P1 defect at this head. Default-off behavior follows the version-skew option in #1965, so obtaining strict HEAD classification still requires enabling the URL option after upgrading the blob servers.
ordishs
approved these changes
Oct 27, 2026
ordishs
left a comment
ordishs
left a comment
Collaborator
There was a problem hiding this comment.
The change is correct and safe for a mixed fleet. The server sets X-Blob-Miss on every miss path that I found. Exists and GetIoReader read the header correctly, and the strict mode stays opt-in. Tests pass with -race.
Follow-ups. I agree with the open points from the other review:
stores/blob/http/http.go:348-355: no test covers the GET header branch. Withmiss := falsein place of the header check, both packages still pass. Please add a case where a 404 carriesX-Blob-Missand a body other thanNotFoundMsg, and the call returnsErrNotFound.stores/blob/http/http.go:159:requireMissHeaderdoes not appear indocs/references/settings/stores/blob_settings.md. Please add it to the URL parameter tables (forhttp://stores, defaultfalse).- Default-off and
Fixes #1965: default-off follows the version-skew option in the issue. But the merge closes the issue while every deployment keeps the oldExistsbehaviour. UseRefs #1965, or open a follow-up issue to flip the default. - Optional,
http.go:254: in compat mode, the Warn fires on every miss from a server without the header. Log at Debug, or log once, while strict mode is off. - Optional,
Del(http.go:543):Delstill checks only the body. If you fold the header check intoisBlobServerMiss,Delgets the header check too and the inline branch inGetIoReadergoes away.
freemans13
approved these changes
Oct 27, 2026
freemans13
left a comment
freemans13
left a comment
Collaborator
There was a problem hiding this comment.
Approve at 357b27f. I found no blocking correctness defect or additional finding beyond the existing feedback.
The server marks its HEAD and shared GET/ranged-GET/PATCH miss paths before writing the status. Strict Exists distinguishes an unmarked 404 from a blob miss, and GET preserves compatibility with the old response body. Default-off follows the client-setting option explicitly proposed in the version-skew section of issue 1965, so I do not consider that default a correctness blocker.
I agree with these follow-ups already raised:
- Add the GET header-only regression case. I disabled the header check through a temporary Go build overlay, and both packages still passed twice under
-race. A 404 withX-Blob-Miss: 1and a body other thanNotFoundMsgmust returnErrNotFound. Existing discussion. - Document
requireMissHeaderindocs/references/settings/stores/blob_settings.md, including its default and when to enable it. Track the future default change separately if this PR closes the issue. Existing review.
Local verification passed on Go 1.27.3, darwin/arm64:
SETTINGS_CONTEXT=test go test -race -tags testtxmetacache -count=2 ./stores/blob/http ./stores/blob
GitHub's test and smoke-test workflows are action_required; they have not run. I did not run the full repository suite or lint locally. The checkout is unchanged.
…uireMissHeader Review follow-ups on the miss-header change. isBlobServerMiss now decides on MissHeader first and falls back to the NotFoundMsg body, so GetIoReader (and through it Get) and Del read a header-marked 404 the same way. The inline header branch in GetIoReader is gone, and Del no longer needs the body to match when the header is present. The body is always drained, so the connection stays reusable. Exists logs a bare 404 at Debug while requireMissHeader is off, because every legitimate miss from a server that predates the header lands there; it stays a Warn when the flag is on and the 404 is returned as a fault. TestHTTPStore_MissHeaderAloneDecidesAMiss answers GET and DELETE with the header and a body that is not NotFoundMsg: the server also sends NotFoundMsg, which had hidden a header branch that never fired. Disabling the header check fails the GetIoReader, Get and Del subtests; the two without-header cases pin that a foreign body alone is still a store fault. requireMissHeader is added to the URL parameter table in blob_settings.md.
icellan
approved these changes
Oct 27, 2026
icellan
left a comment
icellan
left a comment
Contributor
There was a problem hiding this comment.
Both blockers addressed. Nits below are non-blocking.
Nit outside the diff: the doc comments on Exists (~218), GetIoReader (~329) and Del (~513) still describe the old contract. Under requireMissHeader a bare HEAD 404 is a storage error, and the header now decides a miss whatever the body says.
| func isBlobServerMiss(resp *http.Response) bool { | ||
| body, err := io.ReadAll(io.LimitReader(resp.Body, notFoundBodyLimit)) | ||
|
|
||
| // The header decides on its own: it is set on every miss path, whatever the body says. |
Contributor
There was a problem hiding this comment.
Nit: any non-empty value counts, so X-Blob-Miss: 0 from a proxy reads as a miss. The server always sends 1; compare against that in one helper and use it here and in Exists (line 258) instead of two copies.
| case http.StatusNotFound: | ||
| // The blob server marks its own miss with MissHeader on every miss path. A HEAD | ||
| // has no body, so unlike GET this cannot fall back to NotFoundMsg - the header is | ||
| // the only signal that tells the server's miss from a wrong base URL or a proxy |
Contributor
There was a problem hiding this comment.
Nit: the header proves a blob server answered, not the right one. A base URL pointed at another Teranode blob server (e.g. the block store instead of the subtree store) still gets X-Blob-Miss on every key and reads as empty, even with requireMissHeader=true. Worth softening "tells it from a wrong base URL" here and in blob_settings.md.
|
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-10-28 10:21 UTC |
This branch has not been deployed
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.
Refs #1965.
Problem
HTTPStore.Existssends HEAD, and a HEAD response has no body, so it could not use theNotFoundMsgbody-check that #1963 gave GET: it read any 404 as "not held". A wrong base URL or a proxy answering 404 therefore made every blob look absent to callers that branch onExists.Fix
X-Blob-Miss: 1is set on every path that answers a miss:handleExists(the HEAD/Exists path) andwriteStoreError(GET, ranged GET and PATCH, which all route their miss through it).http.Errorsets the status after the header, so it survives.Existsreads a 404 as "not held" only when the header is present. A 404 that carriesX-Blob-Missis a miss; any other 404 is logged (address in the log line, not the error text, as fix(blob/http): keep a blob-store fault from reading as a miss over HTTP #1963 does) and — see skew below — can be surfaced as aStorageError.GetIoReader's miss test is nowheader present || NotFoundMsg body; the||short-circuits so the body is only read when the header is absent. This keeps working against servers that predate the header and makes GET robust to a proxy that echoes aNot found-ish body.Version skew
An old server sends no header. Until every blob server does, a bare HEAD 404 must still read as a miss, so the strict behaviour is opt-in and off by default:
Existstreats a header-less 404 as a miss unless the store URL carries?requireMissHeader=true, in which case it is aStorageError. (The issue also floated advertising the header on/health; the setting is the simpler first step and can become the default once every server ships the header. No shipped config uses an HTTP blob store today, so the skew mostly concerns test harnesses.)The
MissHeaderconstant lives in the client package (stores/blob/http) and the server references it as it already does forNotFoundMsg, so the two cannot drift.Tests
stores/blob/http:TestHTTPStore_ExistsUsesMissHeader— 404+header is not-held/no-error; 200 is held; a bare 404 is a miss by default; a bare 404 is aStorageErrorunder?requireMissHeader=true(and the fault does not name the server). The existingTestHTTPStore_GetMissIsNotFoundstill passes (no case sets the header, so the body-check fallback is unchanged).stores/blob:TestHTTPBlobServer_MarksMissWithHeader— the Exists/HEAD miss and the GET miss both carryX-Blob-Miss.Discrimination: reverting just the behavioural lines (the server's header-set and the client's Exists header-branch), keeping the new symbols, makes both new tests fail — so they pin the behaviour, not vacuously pass.