From 9b7be2c76edb7785a9da939b7ad62849b9cdb0b5 Mon Sep 17 00:00:00 2001 From: vickydotbat Date: Sat, 1 Aug 2026 00:26:22 +0200 Subject: [PATCH] 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) --- README.md | 2 +- internal/nwsync/compressedbuf.go | 21 ++++++++++---- internal/nwsync/manifest.go | 12 ++++++-- internal/nwsync/sink.go | 19 +++++++++---- internal/nwsync/verify.go | 7 +++-- internal/nwsync/verify_test.go | 48 +++++++++++++++++++++++--------- 6 files changed, 79 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index 2495a2d..25c93c5 100644 --- a/README.md +++ b/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 | diff --git a/internal/nwsync/compressedbuf.go b/internal/nwsync/compressedbuf.go index e66267c..133c972 100644 --- a/internal/nwsync/compressedbuf.go +++ b/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, - // matching the field's own encoding threshold. - frameSizeThreshold = 256 + // oneByteContentSizeCeiling is the size above which a Frame_Content_Size no + // longer fits in one byte. Below it the field's size flag is 0, which is + // 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] diff --git a/internal/nwsync/manifest.go b/internal/nwsync/manifest.go index 17ddd4b..089999a 100644 --- a/internal/nwsync/manifest.go +++ b/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))) } diff --git a/internal/nwsync/sink.go b/internal/nwsync/sink.go index d8f5930..19c1d64 100644 --- a/internal/nwsync/sink.go +++ b/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 || blobMatchesName(stored, sha1Hex) == nil { + if !verify { + // 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 - // repairing what is there is the whole point of verifying. + // its own name. + // + // 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) diff --git a/internal/nwsync/verify.go b/internal/nwsync/verify.go index 8c8fdb9..631274a 100644 --- a/internal/nwsync/verify.go +++ b/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" diff --git a/internal/nwsync/verify_test.go b/internal/nwsync/verify_test.go index b44c142..7603151 100644 --- a/internal/nwsync/verify_test.go +++ b/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 "data/sha1/" + hexed[0:2] + "/" + hexed[2:4] + "/" + hexed + return blobKey(hex.EncodeToString(sum[:])) } 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 { - t.Errorf("checked %d blobs, want 3", result.Checked) + // A default run is a full sweep, so it must reach every blob the manifest + // 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 { - t.Errorf("reported %d distinct blobs, want 3", result.Blobs) + if result.Blobs <= result.Checked { + 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)