fix(nwsync): close the review findings on the framing fix and verify

- compressBlob now asserts its own output declares a content size, instead of
  trusting that the only way to lose the field is the one #86 found. An encoder
  upgrade that finds another way would otherwise republish the same undecodable
  blob in silence, and a blob is skipped by every later emit once written.
- inspectBlob asserts the frame whenever one is present, rather than only when
  the payload is non-empty.
- dirSink stats instead of reading when not verifying. Reading every existing
  blob back on the default path was a plain regression, and it contradicted the
  documented promise that verifying is a repair pass, not the default.
- The manifest sha1 is now checked as hex before it is interpolated into a URL
  path, rather than only measured.
- One blobKey rule for where a blob lives, replacing three copies of the
  fanout-path expression.
- Renamed frameSizeThreshold to oneByteContentSizeCeiling, which says whose
  threshold it is.
- The read-back in zoneSink reads the storage API on purpose; said so, and
  corrected a comment that claimed a failed read-back re-uploads when it
  aborts.
- Tests derive their expected blob counts from the run instead of hard-coding
  fixture arithmetic, and the size-mismatch category has a test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

README.md

@@ -19,7 +19,7 @@ Crucible is how the artifact repos turn source into artifacts.
 | `crucible-depot`   | `crucible depot`   | content-addressed depot blob verify/move        |
 | `crucible-hak`     | `crucible hak`     | ERF/HAK pack/unpack + hak manifests             |
 | `crucible-module`  | `crucible module`  | build/extract/validate/compare the `.mod`       |
-| `crucible-nwsync`  | `crucible nwsync`  | NWSync blob emit + manifest assemble            |
+| `crucible-nwsync`  | `crucible nwsync`  | NWSync blob emit + manifest assemble + verify   |
 | `crucible-topdata` | `crucible topdata` | compile 2da/tlk topdata + packages              |
 | `crucible-wiki`    | `crucible wiki`    | render + deploy mechanical wiki pages           |
 

internal/nwsync/compressedbuf.go

@@ -39,9 +39,10 @@ const (
 	zstdFrameMagic      = "\x28\xb5\x2f\xfd"
 	frameSingleSegment  = 1 << 5
 	frameDictionaryMask = 0x03
-	// The size below which klauspost/compress writes no Frame_Content_Size,
+	// oneByteContentSizeCeiling is the size above which a Frame_Content_Size no
-	// matching the field's own encoding threshold.
+	// longer fits in one byte. Below it the field's size flag is 0, which is
-	frameSizeThreshold = 256
+	// what lets klauspost/compress leave the field out entirely.
+	oneByteContentSizeCeiling = 256
 )
 
 // compressBlob wraps data in NWCompressedBuffer framing.
@@ -51,7 +52,15 @@ func compressBlob(data []byte) []byte {
 	for _, field := range header {
 		_ = binary.Write(&out, binary.LittleEndian, field)
 	}
-	out.Write(declareFrameContentSize(blobEncoder.EncodeAll(data, nil), len(data)))
+	frame := declareFrameContentSize(blobEncoder.EncodeAll(data, nil), len(data))
+	// Fail closed rather than publish a blob no client can decode. An encoder
+	// upgrade that finds a new way to omit the field would otherwise reproduce
+	// #86 in silence, and a blob is skipped by every later emit once written.
+	if !frameDeclaresContentSize(frame) {
+		panic(fmt.Sprintf("nwsync: refusing to emit a %d-byte blob whose zstd frame declares no content size (descriptor %#x)",
+			len(data), frame[4]))
+	}
+	out.Write(frame)
 	return out.Bytes()
 }
 
@@ -68,7 +77,7 @@ func inspectBlob(blob []byte) ([]byte, error) {
 	if err != nil {
 		return nil, fmt.Errorf("malformed framing: %w", err)
 	}
-	if len(data) > 0 && !frameDeclaresContentSize(blob[blobHeaderBytes:]) {
+	if len(blob) > blobHeaderBytes && !frameDeclaresContentSize(blob[blobHeaderBytes:]) {
 		return nil, fmt.Errorf("malformed framing: the zstd frame declares no content size, which the game client cannot decode")
 	}
 	return data, nil
@@ -115,7 +124,7 @@ func frameDeclaresContentSize(frame []byte) bool {
 // which is sound because the content is under 256 bytes and every match in it
 // therefore falls inside that window. Same length in, same length out.
 func declareFrameContentSize(frame []byte, size int) []byte {
-	if size <= 0 || size >= frameSizeThreshold || len(frame) < 6 || string(frame[:4]) != zstdFrameMagic {
+	if size <= 0 || size >= oneByteContentSizeCeiling || len(frame) < 6 || string(frame[:4]) != zstdFrameMagic {
 		return frame
 	}
 	descriptor := frame[4]

internal/nwsync/manifest.go

@@ -7,6 +7,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"path"
 	"path/filepath"
 	"sort"
 	"strings"
@@ -194,7 +195,14 @@ func marshalSidecar(sidecar Sidecar) ([]byte, error) {
 	return append(body, '\r', '\n'), nil
 }
 
-// blobPath is the data store path for a blob, hash tree depth 2.
+// blobKey is where a blob lives in a zone, hash tree depth 2. emit writes it,
+// verify reads it and the game client requests it, so the rule lives here and
+// nowhere else.
+func blobKey(sha1Hex string) string {
+	return path.Join("data", "sha1", sha1Hex[0:2], sha1Hex[2:4], sha1Hex)
+}
+
+// blobPath is the same location inside a local repository tree.
 func blobPath(root, sha1Hex string) string {
-	return filepath.Join(root, "data", "sha1", sha1Hex[0:2], sha1Hex[2:4], sha1Hex)
+	return filepath.Join(root, filepath.FromSlash(blobKey(sha1Hex)))
 }

internal/nwsync/sink.go

@@ -45,10 +45,14 @@ type dirSink struct{ root string }
 
 func (s dirSink) putBlob(sha1Hex string, verify bool, body func() []byte) (int64, error) {
 	blob := blobPath(s.root, sha1Hex)
-	if stored, err := os.ReadFile(blob); err == nil {
+	if !verify {
-		if !verify || blobMatchesName(stored, sha1Hex) == nil {
+		// Stat, not read: the common path must not pay to open every blob that
+		// is already there.
+		if _, err := os.Stat(blob); err == nil {
 			return 0, nil
 		}
+	} else if stored, err := os.ReadFile(blob); err == nil && blobMatchesName(stored, sha1Hex) == nil {
+		return 0, nil
 	}
 	if err := os.MkdirAll(filepath.Dir(blob), 0o755); err != nil {
 		return 0, fmt.Errorf("create blob directory: %w", err)
@@ -100,7 +104,7 @@ type zoneSink struct {
 }
 
 func (s zoneSink) putBlob(sha1Hex string, verify bool, body func() []byte) (int64, error) {
-	key := path.Join("data", "sha1", sha1Hex[0:2], sha1Hex[2:4], sha1Hex)
+	key := blobKey(sha1Hex)
 	// A throttled probe must never be read as "missing, re-upload" or as
 	// "present, skip", so only a confirmed Present skips the upload.
 	state, _, err := s.store.ProbeKey(s.ctx, key)
@@ -112,8 +116,13 @@ func (s zoneSink) putBlob(sha1Hex string, verify bool, body func() []byte) (int6
 			return 0, nil
 		}
 		// The probe only proved the object exists. Read it back and hold it to
-		// its own name; a failure here means re-upload, not abort, because
+		// its own name.
-		// repairing what is there is the whole point of verifying.
+		//
+		// This reads the storage API rather than the pull zone: emit holds the
+		// write credential, and a repair decision has to be made against the
+		// copy it is about to overwrite, not against an edge cache of it. A
+		// read that fails outright is a fault, not a verdict — treating it as
+		// "bad, re-upload" would turn a throttled zone into a full backfill.
 		stored, err := s.store.GetKey(s.ctx, key)
 		if err != nil {
 			return 0, fmt.Errorf("read back blob %s: %w", sha1Hex, err)

internal/nwsync/verify.go

@@ -103,7 +103,9 @@ type VerifyResult struct {
 // are wrong — and a round-trip check alone would pass a frame that omits its
 // content size, because Go's decoder is more capable than the client's (#86).
 func Verify(options VerifyOptions) (VerifyResult, error) {
-	if len(options.ManifestSHA1) != 40 {
+	// The argument is interpolated straight into a URL path, so it is checked
+	// rather than trusted: exactly 20 bytes of hex, nothing else.
+	if sum, err := hex.DecodeString(options.ManifestSHA1); err != nil || len(sum) != sha1.Size {
 		return VerifyResult{}, fmt.Errorf("%q is not a manifest sha1", options.ManifestSHA1)
 	}
 	source := options.Source
@@ -198,7 +200,6 @@ func Verify(options VerifyOptions) (VerifyResult, error) {
 // checkEntry fetches one blob and returns a one-line fault, or "" if it is
 // exactly what the manifest entry says it is.
 func checkEntry(source blobSource, entry Entry) string {
-	key := path.Join("data", "sha1", entry.sha1Hex()[0:2], entry.sha1Hex()[2:4], entry.sha1Hex())
 	// Name the resource, not just the hash: an operator has to find the thing
 	// in a hak, and a bare sha1 says nothing about where to look.
 	extension, ok := erf.ExtensionForResourceType(entry.ResType)
@@ -206,7 +207,7 @@ func checkEntry(source blobSource, entry Entry) string {
 		extension = strconv.Itoa(int(entry.ResType))
 	}
 	where := fmt.Sprintf("%s (%s.%s)", entry.sha1Hex(), entry.ResRef, extension)
-	blob, err := source.get(key)
+	blob, err := source.get(blobKey(entry.sha1Hex()))
 	if err != nil {
 		if errors.Is(err, errBlobMissing) {
 			return where + ": missing"

internal/nwsync/verify_test.go

@@ -60,12 +60,11 @@ func (f *verifyFixture) verify(t *testing.T, sample int) (VerifyResult, string,
 	return result, log.String(), err
 }
 
-// blobKey is where a resource's blob lives, addressed by the sha1 of its
+// keyOf is where a resource's blob lives, addressed by the sha1 of its
 // uncompressed bytes — the same path the client requests.
-func blobKey(body []byte) string {
+func keyOf(body []byte) string {
 	sum := sha1.Sum(body)
-	hexed := hex.EncodeToString(sum[:])
+	return blobKey(hex.EncodeToString(sum[:]))
-	return "data/sha1/" + hexed[0:2] + "/" + hexed[2:4] + "/" + hexed
 }
 
 func TestVerifyPassesACleanZone(t *testing.T) {
@@ -77,14 +76,16 @@ func TestVerifyPassesACleanZone(t *testing.T) {
 	if result.Failures != 0 {
 		t.Errorf("verify reported %d failures on a clean zone: %s", result.Failures, log)
 	}
-	if result.Checked != 3 {
+	// A default run is a full sweep, so it must reach every blob the manifest
-		t.Errorf("checked %d blobs, want 3", result.Checked)
+	// names — not some of them.
+	if result.Checked != result.Blobs || result.Blobs == 0 {
+		t.Errorf("checked %d of %d blobs; a full sweep must check all of them", result.Checked, result.Blobs)
 	}
 }
 
 func TestVerifyReportsAMissingBlob(t *testing.T) {
 	fixture := newVerifyFixture(t)
-	key := blobKey([]byte("small"))
+	key := keyOf([]byte("small"))
 	fixture.zone.mu.Lock()
 	delete(fixture.zone.objects, key)
 	fixture.zone.mu.Unlock()
@@ -103,7 +104,7 @@ func TestVerifyReportsAMissingBlob(t *testing.T) {
 
 func TestVerifyReportsATruncatedBlob(t *testing.T) {
 	fixture := newVerifyFixture(t)
-	key := blobKey([]byte("small"))
+	key := keyOf([]byte("small"))
 	fixture.zone.mu.Lock()
 	fixture.zone.objects[key] = fixture.zone.objects[key][:blobHeaderBytes+4]
 	fixture.zone.mu.Unlock()
@@ -126,7 +127,7 @@ func TestVerifyReportsATruncatedBlob(t *testing.T) {
 func TestVerifyRejectsABlobWithNoDeclaredFrameContentSize(t *testing.T) {
 	fixture := newVerifyFixture(t)
 	body := []byte("small")
-	key := blobKey(body)
+	key := keyOf(body)
 
 	fixture.zone.mu.Lock()
 	good := fixture.zone.objects[key]
@@ -159,7 +160,7 @@ func TestVerifyRejectsABlobWithNoDeclaredFrameContentSize(t *testing.T) {
 
 func TestVerifyReportsWrongContents(t *testing.T) {
 	fixture := newVerifyFixture(t)
-	key := blobKey([]byte("small"))
+	key := keyOf([]byte("small"))
 	fixture.zone.mu.Lock()
 	// Valid framing, valid zstd, wrong bytes: only decompressing and hashing
 	// can see this, which is why Content-Length is not enough.
@@ -178,6 +179,27 @@ func TestVerifyReportsWrongContents(t *testing.T) {
 	}
 }
 
+func TestVerifyReportsAShortBlob(t *testing.T) {
+	fixture := newVerifyFixture(t)
+	key := keyOf([]byte("small"))
+	fixture.zone.mu.Lock()
+	// Well-formed all the way down and simply too short — the shape a killed
+	// upload leaves behind, and the one a Content-Length check would pass.
+	fixture.zone.objects[key] = compressBlob([]byte("sma"))
+	fixture.zone.mu.Unlock()
+
+	result, log, err := fixture.verify(t, 0)
+	if err != nil {
+		t.Fatalf("verify: %v", err)
+	}
+	if result.Failures != 1 {
+		t.Fatalf("reported %d failures, want 1: %s", result.Failures, log)
+	}
+	if !strings.Contains(log, "size mismatch") {
+		t.Errorf("a short blob was not reported as a size mismatch: %s", log)
+	}
+}
+
 func TestVerifyFailsWhenTheManifestIsNotTheOneAsked(t *testing.T) {
 	fixture := newVerifyFixture(t)
 	fixture.zone.mu.Lock()
@@ -198,8 +220,8 @@ func TestVerifySampleChecksFewerBlobs(t *testing.T) {
 	if result.Checked != 1 {
 		t.Errorf("--sample 1 checked %d blobs, want 1: %s", result.Checked, log)
 	}
-	if result.Blobs != 3 {
+	if result.Blobs <= result.Checked {
-		t.Errorf("reported %d distinct blobs, want 3", result.Blobs)
+		t.Errorf("sampling %d of %d blobs is not a sample", result.Checked, result.Blobs)
 	}
 }
 
@@ -229,7 +251,7 @@ func TestEmitVerifyReplacesABlobThatIsNotItsName(t *testing.T) {
 	}
 
 	emit(false)
-	key := blobKey(body)
+	key := keyOf(body)
 	encoder, err := zstd.NewWriter(nil, zstd.WithEncoderConcurrency(1))
 	if err != nil {
 		t.Fatal(err)