fix(nwsync): declare Frame_Content_Size on every blob, and verify what is published (#87)
build-binaries / build-binaries (push) Successful in 2m40s
build-binaries / build-binaries (push) Successful in 2m40s
Closes #86. Closes #85. These land together on purpose. Fixing the encoder alone changes nothing for the blobs already in the zone, because `emit` skips whatever is already present. ## #86 — the framing fix `klauspost/compress` omits the zstd `Frame_Content_Size` field for inputs under 256 bytes, which the format permits. Reference libzstd never does, so the NWN client — which sizes its output buffer from `ZSTD_getFrameContentSize` and has therefore never met a frame without one — rejected roughly 6% of our blobs outright. Any single one stops a sync dead, so no client could complete a sync of the live manifest. No encoder option changes this, so `compressBlob` re-headers the affected frames into the shape libzstd itself emits: `Single_Segment_flag` set, `Window_Descriptor` dropped, and the freed byte spent on a one-byte `Frame_Content_Size`. Same length in, same length out, and the same descriptor byte (`0x24`) the issue recorded from libzstd. `compressBlob` then asserts its own output. An encoder upgrade that finds another way to omit the field would otherwise reproduce #86 in silence, and a blob is skipped by every later emit once written. `emitter_version` goes to `2`, so `assemble` refuses to merge an index written by the encoder that omitted the field. **Proved against the reference decoder, not just a round trip.** A real emitted 175-byte blob: ``` Frames Skips Compressed Uncompressed Ratio Check Filename 1 0 48 B 175 B 3.646 XXH64 frame.zst c59d6620d4ffd4bf3fe73df43b19b7afcfe8fea4 - <- zstd -dc | sha1sum c59d6620d4ffd4bf3fe73df43b19b7afcfe8fea4 <- the blob's own name ``` Before the fix that `Uncompressed` column was blank. ## #85 — `nwsync verify` `crucible nwsync verify <manifest-sha1>` reads a manifest and its blobs back through the **public pull zone**, with no credential, because what matters is the bytes a client is served, edge behaviour included. Every distinct blob is decompressed and hashed; failures are reported per blob as missing / malformed framing / size mismatch / hash mismatch, and the exit code is 1. - `--sample N` makes a routine check cheap against a manifest that is ~69,000 blobs and 15 GB; the default is a full sweep. - `--base URL` / `NWSYNC_PULL_BASE` overrides the public host. - The manifest is checked against its own sha1 before a single blob is fetched. - `emit --verify` applies the same check where `emit` would otherwise trust presence, and replaces a stored blob that is not what its name claims. This is what makes the #86 blobs repairable. ## Why the existing checks missed this Both new checks assert the **frame property**, not just a round trip. The conformance suite (#59) compares decompressed bytes, so a frame that decodes correctly passes regardless of its header; and the earlier zone audit decompressed 68 blobs with the `zstd` CLI, a *more* capable decoder than the client's, which certified exactly the blobs the client rejects. ## Checks `make check` and `make smoke` green. Second commit is the fixes from a two-axis review of the first. ## Not in this PR Three follow-ups, filed separately: the backfill has not been run, replacing a blob does not purge the pull-zone edge cache, and #85's runbook line belongs to `sow-platform`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)Reviewed-on: #87 Co-authored-by: vickydotbat <vickydotbat@tutamail.com>
This commit was merged in pull request #87.
This commit is contained in:
+39
-10
@@ -20,11 +20,17 @@ import (
|
||||
// upstream's output and ours can be diffed on a developer machine.
|
||||
type sink interface {
|
||||
// putBlob stores one NWCompressedBuffer blob under its sha1 name and
|
||||
// returns the bytes stored, or 0 if the blob was already there. Blob names
|
||||
// are content hashes, so an existing name is existing content — which is
|
||||
// why body is a thunk: compression is the expensive part of emit and a
|
||||
// blob that is already stored must not pay for it.
|
||||
putBlob(sha1Hex string, body func() []byte) (int64, error)
|
||||
// returns the bytes stored, or 0 if a good copy was already there. Blob
|
||||
// names are content hashes, so an existing name is normally taken as
|
||||
// existing content — which is why body is a thunk: compression is the
|
||||
// expensive part of emit and a blob that is already stored must not pay
|
||||
// for it.
|
||||
//
|
||||
// verify stops trusting presence: the stored copy is read back, unwrapped
|
||||
// and hashed, and replaced when it is not what its name claims. Without it
|
||||
// an object written truncated, or written by an emitter since found broken,
|
||||
// is skipped by every later emit forever and no backfill can repair it.
|
||||
putBlob(sha1Hex string, verify bool, body func() []byte) (int64, error)
|
||||
// putIndex stores a NSYM manifest and its sidecar under key, which is
|
||||
// either an artifact-derived object key or a local path.
|
||||
putIndex(key string, manifest, sidecar []byte) error
|
||||
@@ -37,9 +43,15 @@ type sink interface {
|
||||
// dirSink writes a local NWSync repository tree.
|
||||
type dirSink struct{ root string }
|
||||
|
||||
func (s dirSink) putBlob(sha1Hex string, body func() []byte) (int64, error) {
|
||||
func (s dirSink) putBlob(sha1Hex string, verify bool, body func() []byte) (int64, error) {
|
||||
blob := blobPath(s.root, sha1Hex)
|
||||
if _, err := os.Stat(blob); err == 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 {
|
||||
@@ -91,8 +103,8 @@ type zoneSink struct {
|
||||
zone string
|
||||
}
|
||||
|
||||
func (s zoneSink) putBlob(sha1Hex string, body func() []byte) (int64, error) {
|
||||
key := path.Join("data", "sha1", sha1Hex[0:2], sha1Hex[2:4], sha1Hex)
|
||||
func (s zoneSink) putBlob(sha1Hex string, verify bool, body func() []byte) (int64, error) {
|
||||
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)
|
||||
@@ -100,7 +112,24 @@ func (s zoneSink) putBlob(sha1Hex string, body func() []byte) (int64, error) {
|
||||
return 0, fmt.Errorf("probe blob %s: %w", sha1Hex, err)
|
||||
}
|
||||
if state == depot.Present {
|
||||
return 0, nil
|
||||
if !verify {
|
||||
return 0, nil
|
||||
}
|
||||
// The probe only proved the object exists. Read it back and hold it to
|
||||
// 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)
|
||||
}
|
||||
if blobMatchesName(stored, sha1Hex) == nil {
|
||||
return 0, nil
|
||||
}
|
||||
}
|
||||
data := body()
|
||||
if err := s.put(key, data); err != nil {
|
||||
|
||||
Reference in New Issue
Block a user