Compare commits

..
2 Commits
Author SHA1 Message Date
archvillainetteandClaude Opus 5 9b7be2c76e fix(nwsync): close the review findings on the framing fix and verify
ci / ci (pull_request) Successful in 3m38s
- 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>
2026-08-01 00:26:22 +02:00
archvillainetteandClaude Opus 5 56d4054118 fix(nwsync): declare Frame_Content_Size, and verify what is published (#86, #85)
These two 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 — 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 emit 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. Verified against the zstd CLI end to end: a real emitted 230-byte
blob now reports its decompressed size where it previously reported none.
emitter_version goes to 2, so assemble refuses to merge an index written by the
encoder that omitted the field.

#85 — a published blob was verified by exactly one thing, a player's client, at
download time. `nwsync verify <manifest-sha1>` now reads a manifest and its
blobs back through the public pull zone with no credential, decompresses and
hashes every one, and reports missing / malformed framing / size mismatch / hash
mismatch per blob. `--sample N` makes a routine check cheap against a manifest
that is ~69,000 blobs and 15 GB. `emit --verify` applies the same check where
emit would otherwise trust presence, and replaces a stored blob that is not what
its name claims — which is what makes the #86 blobs repairable.

Both new checks assert the frame property, not just a round trip. Round-tripping
cannot see this class of fault: both the zstd CLI and Go's decoder stream such a
frame happily, being more capable decoders than the client's, which is how the
defect reached production and survived an audit.

Refs #54, #55, #59, #75, sow-platform#79.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-01 00:20:15 +02:00
4 changed files with 2 additions and 104 deletions
-17
View File
@@ -97,23 +97,6 @@ broken — is skipped by every later run forever and no backfill repairs it. Wit
and replaced when it does not match. It costs a full GET per existing blob, so and replaced when it does not match. It costs a full GET per existing blob, so
it is a repair pass, not the default. it is a repair pass, not the default.
**After a repair, `verify` is what tells you which keys to purge.** A repair is
the one thing that makes a key serve different bytes than it did before, and the
edge caches these objects for 30 days precisely because that normally cannot
happen. The two commands look at different copies on purpose: `emit --verify`
repairs the **origin**, `verify` reads the **edge**. So a `verify` run straight
after a repair is not a verdict — it is a survey, and every blob it still calls
bad is one the edge is serving stale. Purge exactly those, then re-run it; only
that second run is the verdict.
Purging the keys `verify` names beats purging the zone, because the edge only
ever cached what somebody actually fetched: the 2026-08-01 repair rewrote 2,603
blobs at the origin and left 8 stale at the edge. The purge belongs in the
repair procedure rather than in `emit`, which reports how many blobs it wrote
and never which ones — so it could not target one even with a CDN credential,
which it deliberately does not hold (#89; the procedure itself is in
sow-platform's NWSync runbook).
`emit` uploads blobs first and the index last, so the presence of an index is `emit` uploads blobs first and the index last, so the presence of an index is
the publication marker: an artifact whose emit died halfway leaves real blobs in the publication marker: an artifact whose emit died halfway leaves real blobs in
the zone and no index. Blob names are content hashes, so re-running skips the zone and no index. Blob names are content hashes, so re-running skips
-3
View File
@@ -67,9 +67,6 @@ check on a published blob upstream of a player's client.
--verify makes emit hash what it would otherwise skip. emit normally treats a --verify makes emit hash what it would otherwise skip. emit normally treats a
blob's presence as proof of its contents, so without this an object written blob's presence as proof of its contents, so without this an object written
truncated, or written by an emitter since found broken, is skipped forever. truncated, or written by an emitter since found broken, is skipped forever.
--verify repairs the storage zone, while verify reads the edge in front of it.
So a verify run right after a repair is a survey, not a verdict: it names the
keys the edge still serves stale. Purge those, then run it again.
--out DIR writes to a local repository tree instead of uploading, which is the --out DIR writes to a local repository tree instead of uploading, which is the
conformance path against upstream nwn_nwsync_write. Without it, the zone comes conformance path against upstream nwn_nwsync_write. Without it, the zone comes
+2 -12
View File
@@ -560,18 +560,8 @@ func (p *Project) ValidateLayout() error {
if strings.TrimSpace(p.Config.Module.ResRef) == "" { if strings.TrimSpace(p.Config.Module.ResRef) == "" {
failures = append(failures, errors.New("module.resref is required")) failures = append(failures, errors.New("module.resref is required"))
} }
// module.resref names the built .mod FILE, so the 16-byte resref limit does not if len(p.Config.Module.ResRef) > 16 {
// apply to it — NWN:EE module file names are routinely longer. It is validated as failures = append(failures, fmt.Errorf("module.resref %q exceeds 16 characters", p.Config.Module.ResRef))
// a file name instead. The limit still binds when the same value has to be a real
// resref: with no haks configured, an asset project names its single generated HAK
// after it, and a HAK name is a resref the engine loads.
if err := validateOutputFileName("module.resref", p.Config.Module.ResRef+".mod", ".mod"); err != nil {
failures = append(failures, err)
}
if len(p.Config.Module.ResRef) > 16 && strings.TrimSpace(p.Config.Paths.Assets) != "" && len(p.Config.HAKs) == 0 {
failures = append(failures, fmt.Errorf(
"module.resref %q exceeds 16 characters and would name this project's generated HAK; configure haks[] with a shorter name",
p.Config.Module.ResRef))
} }
if strings.TrimSpace(p.Config.Paths.Source) == "" && strings.TrimSpace(p.Config.Paths.Assets) == "" && !p.HasTopData() { if strings.TrimSpace(p.Config.Paths.Source) == "" && strings.TrimSpace(p.Config.Paths.Assets) == "" && !p.HasTopData() {
failures = append(failures, errors.New("at least one of paths.source, paths.assets, or topdata.source is required")) failures = append(failures, errors.New("at least one of paths.source, paths.assets, or topdata.source is required"))
-72
View File
@@ -1093,78 +1093,6 @@ func TestValidateLayoutAllowsMissingAssetsDir(t *testing.T) {
} }
} }
// module.resref names the built .mod FILE, not a resource inside an archive, so the
// 16-byte resref limit does not apply to it. NWN:EE module file names are commonly
// longer (ShadowsOverWestgate.mod is 19). The limit still binds everywhere a resref
// really is a resref — see TestValidateLayoutRejectsLongResRefWhenItNamesAHAK.
func TestValidateLayoutAllowsLongModuleResRef(t *testing.T) {
root := t.TempDir()
mkdirAll(t, filepath.Join(root, "src"))
mkdirAll(t, filepath.Join(root, "build"))
proj := &Project{
Root: root,
Config: Config{
Module: ModuleConfig{Name: "Shadows Over Westgate", ResRef: "ShadowsOverWestgate"},
Paths: PathConfig{Source: "src", Build: "build"},
},
}
if err := proj.ValidateLayout(); err != nil {
t.Fatalf("ValidateLayout rejected a 19-character module file name: %v", err)
}
if got, want := filepath.Base(proj.ModuleArchivePath()), "ShadowsOverWestgate.mod"; got != want {
t.Fatalf("ModuleArchivePath() = %q, want %q", got, want)
}
}
// A module.resref that is not a usable file name is still rejected.
func TestValidateLayoutRejectsModuleResRefThatIsAPath(t *testing.T) {
root := t.TempDir()
mkdirAll(t, filepath.Join(root, "src"))
proj := &Project{
Root: root,
Config: Config{
Module: ModuleConfig{Name: "Test", ResRef: "../escape/mod"},
Paths: PathConfig{Source: "src", Build: "build"},
},
}
err := proj.ValidateLayout()
if err == nil {
t.Fatal("ValidateLayout accepted a module.resref containing a path")
}
if !strings.Contains(err.Error(), "module.resref") {
t.Fatalf("error does not name the offending field: %v", err)
}
}
// When a project declares no haks, the module resref becomes the name of the single
// generated HAK — and a HAK name IS a resref the engine loads. The limit applies
// there, so a long name is only allowed for projects that build no HAKs.
func TestValidateLayoutRejectsLongResRefWhenItNamesAHAK(t *testing.T) {
root := t.TempDir()
mkdirAll(t, filepath.Join(root, "src"))
mkdirAll(t, filepath.Join(root, "assets"))
proj := &Project{
Root: root,
Config: Config{
Module: ModuleConfig{Name: "Shadows Over Westgate", ResRef: "ShadowsOverWestgate"},
Paths: PathConfig{Source: "src", Assets: "assets", Build: "build"},
},
}
err := proj.ValidateLayout()
if err == nil {
t.Fatal("ValidateLayout accepted a 19-character name for a generated HAK")
}
if !strings.Contains(err.Error(), "16") {
t.Fatalf("error does not explain the resref limit: %v", err)
}
}
// paths.build is an OUTPUT dir the builder creates (MkdirAll) before writing, so // paths.build is an OUTPUT dir the builder creates (MkdirAll) before writing, so
// a bare clone with no build dir yet must still validate/build with no pre-step // a bare clone with no build dir yet must still validate/build with no pre-step
// (R2/parity). Only a build path that exists but is not a directory is an error. // (R2/parity). Only a build path that exists but is not a directory is an error.