fix(vmafx-tune-go): deep bug audit — 5 fixes (JSON NaN, subprocess hang, codec cache)

changelog.d/fixed/0979-vmafx-tune-go-deep-bug-audit.md

@@ -0,0 +1,31 @@
+- `vmafx-tune-go` deep bug audit (5 fixes):
+  - **JSON NaN propagation in `bisect_samples`**: `pkg/report.EmitJSON`
+    and `cmd/vmafx-tune/cmd.emitSweepJSON` previously sanitised
+    only the top-level row floats, leaving `bisect.Sample` fields
+    (`bitrate_kbps`, `vmaf_score`, `encode_time_ms`) as raw `float64`
+    in the JSON wire shape. A single non-finite value (e.g. from a
+    corrupt vmaf XML mean) crashed `json.MarshalIndent` with
+    `"unsupported value: NaN"` and broke the Python ↔ Go parser-parity
+    invariant (AGENTS.md #2). Introduces `report.SanitizeBisectSamples`
+    which coerces nested floats to JSON `null`; mirrored in the
+    ladder emitter (Cloud + Hull + Renditions all sanitised across
+    `BitratekBps`, `VMAF`, `TargetVMAF`).
+  - **`parseVMAFXMLMean` accepts the literal "NaN" / "±Inf"**:
+    `strconv.ParseFloat` returns NaN without error for those tokens.
+    The parser now rejects non-finite means at the source so the bisect
+    step records a score failure rather than propagating a corrupt
+    score into the rest of the pipeline.
+  - **Subprocess hang risk (`ffmpeg`, `vmaf`, `ffprobe`)**: every
+    `exec.Command` in `pkg/encoder` and `pkg/bisect` is now
+    `exec.CommandContext` with a per-call timeout. Defaults:
+    `60m` ffmpeg encode, `30m` vmaf score, `30s` ffprobe bitrate
+    probe, `30s` codec discovery. Overridable via
+    `VMAFX_TUNE_ENCODE_TIMEOUT`, `VMAFX_TUNE_SCORE_TIMEOUT`,
+    `VMAFX_TUNE_PROBE_TIMEOUT`.
+  - **Codec discovery cache silently returned stale results on
+    binary-path change**: the previous `sync.Once`-based cache
+    locked in whichever ffmpeg path was probed first, with a
+    `_ = ffmpegBin` no-op pretending to invalidate the cache. After
+    the fix the cache key is the binary path; calling with a
+    different path triggers a re-probe and replaces the cache.
+    `RefreshCodecCache` now updates the cache key alongside the map.

cmd/vmafx-tune/cmd/compare.go

@@ -241,15 +241,15 @@ func runCompare(flags *compareFlags) error {
 // failRow builds a failed Row for an encoder that could not be run.
 func failRow(codec string, target float64, ffmpegBin, reason string) report.Row {
 	return report.Row{
-		Codec:       codec,
-		FFmpegBin:   ffmpegBin,
-		BestCRF:     -1,
-		BitratekBps: math.NaN(),
+		Codec:        codec,
+		FFmpegBin:    ffmpegBin,
+		BestCRF:      -1,
+		BitratekBps:  math.NaN(),
 		EncodeTimeMS: math.NaN(),
-		VMAFScore:   math.NaN(),
-		TargetVMAF:  target,
-		OK:          false,
-		Error:       reason,
+		VMAFScore:    math.NaN(),
+		TargetVMAF:   target,
+		OK:           false,
+		Error:        reason,
 	}
 }
 
@@ -300,27 +300,31 @@ func sortRows(rows []report.Row) {
 	})
 }
 
-// emitSweepJSON emits a schema-v2 sweep JSON payload.
+// emitSweepJSON emits a schema-v2 sweep JSON payload. NaN / Inf float
+// values (including those nested inside bisect_samples) are coerced to
+// JSON null via report.SanitizeBisectSamples; a single corrupt vmaf-XML
+// mean value would otherwise crash json.MarshalIndent and break the
+// Python ↔ Go parser-parity invariant (AGENTS.md #2).
 func emitSweepJSON(
 	results []pairResult,
 	flags *compareFlags,
 	wallTimeMS float64,
 ) (string, error) {
 	// Build a flat payload matching Python SweepReport schema-v2.
 	type wireRow struct {
-		Codec          string          `json:"codec"`
-		Adapter        string          `json:"adapter"`
-		RuntimeVariant string          `json:"runtime_variant"`
-		FFmpegBin      string          `json:"ffmpeg_bin"`
-		EncoderVersion string          `json:"encoder_version"`
-		BestCRF        int             `json:"best_crf"`
-		BitratekBps    any             `json:"bitrate_kbps"`
-		EncodeTimeMS   any             `json:"encode_time_ms"`
-		VMAFScore      any             `json:"vmaf_score"`
-		TargetVMAF     float64         `json:"target_vmaf"`
-		OK             bool            `json:"ok"`
-		Error          string          `json:"error"`
-		BisectSamples  []bisect.Sample `json:"bisect_samples,omitempty"`
+		Codec          string  `json:"codec"`
+		Adapter        string  `json:"adapter"`
+		RuntimeVariant string  `json:"runtime_variant"`
+		FFmpegBin      string  `json:"ffmpeg_bin"`
+		EncoderVersion string  `json:"encoder_version"`
+		BestCRF        int     `json:"best_crf"`
+		BitratekBps    any     `json:"bitrate_kbps"`
+		EncodeTimeMS   any     `json:"encode_time_ms"`
+		VMAFScore      any     `json:"vmaf_score"`
+		TargetVMAF     float64 `json:"target_vmaf"`
+		OK             bool    `json:"ok"`
+		Error          string  `json:"error"`
+		BisectSamples  []any   `json:"bisect_samples,omitempty"`
 	}
 	nan2null := func(v float64) any {
 		if math.IsNaN(v) || math.IsInf(v, 0) {
@@ -343,7 +347,7 @@ func emitSweepJSON(
 			TargetVMAF:     r.row.TargetVMAF,
 			OK:             r.row.OK,
 			Error:          r.row.Error,
-			BisectSamples:  r.row.BisectSamples,
+			BisectSamples:  report.SanitizeBisectSamples(r.row.BisectSamples),
 		}
 	}
 	payload := struct {

cmd/vmafx-tune/cmd/ladder.go

@@ -261,21 +261,21 @@ type ladderWirePayload struct {
 }
 
 // emitLadderJSON serialises the ladder result as pretty-printed JSON.
-// NaN / Inf bitrate fields are coerced to null (RFC 8259 compliance).
+// NaN / Inf bitrate and VMAF fields are coerced to 0 across the entire
+// payload (Cloud, Hull, Renditions) so json.MarshalIndent never aborts
+// with "unsupported value: NaN". A single corrupt vmaf-XML mean value can
+// otherwise propagate through bisect.Run into a Point and crash the whole
+// emitter — breaking the Python ↔ Go parser-parity invariant
+// (AGENTS.md #2). Failed points carry OK=false and an Error string,
+// preserving the lossless schema-v1 contract.
 func emitLadderJSON(
 	result ladder.LadderResult,
 	flags *ladderFlags,
 	wallTimeMS float64,
 ) (string, error) {
-	// Sanitise cloud points: NaN bitrates → 0 (ladder JSON does not use the
-	// report.Row NaN convention since failed points carry OK=false).
-	cloud := make([]ladder.Point, len(result.Cloud))
-	for i, p := range result.Cloud {
-		if math.IsNaN(p.BitratekBps) || math.IsInf(p.BitratekBps, 0) {
-			p.BitratekBps = 0
-		}
-		cloud[i] = p
-	}
+	cloud := sanitizeLadderPoints(result.Cloud)
+	hull := sanitizeLadderPoints(result.Hull)
+	renditions := sanitizeLadderRenditions(result.Renditions)
 
 	resStrs := make([]string, len(flags.resolutions))
 	copy(resStrs, flags.resolutions)
@@ -289,8 +289,8 @@ func emitLadderJSON(
 		ToolVersion:   report.ToolVersion,
 		WallTimeMS:    wallTimeMS,
 		Cloud:         cloud,
-		Hull:          result.Hull,
-		Renditions:    result.Renditions,
+		Hull:          hull,
+		Renditions:    renditions,
 	}
 	b, err := json.MarshalIndent(payload, "", "  ")
 	if err != nil {
@@ -299,6 +299,42 @@ func emitLadderJSON(
 	return string(b) + "\n", nil
 }
 
+// sanitizeFinite returns v when finite, else 0. The ladder JSON convention
+// is to coerce non-finite floats to 0 (with the OK=false flag carrying the
+// failure signal) rather than to JSON null, matching the existing schema-v1
+// contract that fields are always numeric.
+func sanitizeFinite(v float64) float64 {
+	if math.IsNaN(v) || math.IsInf(v, 0) {
+		return 0
+	}
+	return v
+}
+
+// sanitizeLadderPoints returns a copy of pts with all non-finite floats
+// (BitratekBps, VMAF) coerced to 0.
+func sanitizeLadderPoints(pts []ladder.Point) []ladder.Point {
+	out := make([]ladder.Point, len(pts))
+	for i, p := range pts {
+		p.BitratekBps = sanitizeFinite(p.BitratekBps)
+		p.VMAF = sanitizeFinite(p.VMAF)
+		p.TargetVMAF = sanitizeFinite(p.TargetVMAF)
+		out[i] = p
+	}
+	return out
+}
+
+// sanitizeLadderRenditions returns a copy of rs with all non-finite floats
+// coerced to 0.
+func sanitizeLadderRenditions(rs []ladder.Rendition) []ladder.Rendition {
+	out := make([]ladder.Rendition, len(rs))
+	for i, r := range rs {
+		r.BitratekBps = sanitizeFinite(r.BitratekBps)
+		r.VMAF = sanitizeFinite(r.VMAF)
+		out[i] = r
+	}
+	return out
+}
+
 // emitLadderMarkdown renders the ladder result as a Markdown document.
 func emitLadderMarkdown(
 	result ladder.LadderResult,

cmd/vmafx-tune/cmd/ladder_nan_test.go

@@ -0,0 +1,77 @@
+// Copyright 2026 Lusoris
+// SPDX-License-Identifier: BSD-3-Clause-Plus-Patent OR MIT
+//
+// cmd/vmafx-tune/cmd/ladder_nan_test.go — regression coverage for the
+// ladder emitter's NaN/Inf handling. The prior emitLadderJSON only
+// sanitised Point.BitratekBps in Cloud — not Point.VMAF, not the Hull
+// slice, not the Renditions slice. A NaN that reached any of those
+// would crash json.MarshalIndent and abort the whole ladder report,
+// breaking the Python ↔ Go parser-parity invariant (AGENTS.md #2).
+
+package cmd
+
+import (
+	"encoding/json"
+	"math"
+	"strings"
+	"testing"
+
+	"github.com/VMAFx/vmafx/pkg/ladder"
+)
+
+func TestEmitLadderJSON_NaNInHullAndRenditions(t *testing.T) {
+	t.Parallel()
+	flags := &ladderFlags{
+		reference:   "src.mp4",
+		resolutions: []string{"640x480"},
+		targets:     []float64{85.0},
+	}
+	// Construct a result where every slice (Cloud, Hull, Renditions)
+	// contains at least one non-finite value across every float field.
+	res := ladder.LadderResult{
+		Src:     "src.mp4",
+		Encoder: "libx264",
+		Cloud: []ladder.Point{
+			{Width: 640, Height: 480, BitratekBps: math.NaN(), VMAF: 90.0, CRF: 28, TargetVMAF: 85.0, OK: false},
+			{Width: 640, Height: 480, BitratekBps: 800.0, VMAF: math.Inf(1), CRF: 28, TargetVMAF: math.NaN(), OK: false},
+		},
+		Hull: []ladder.Point{
+			{Width: 640, Height: 480, BitratekBps: math.NaN(), VMAF: math.NaN(), CRF: 28, TargetVMAF: 85.0, OK: true},
+		},
+		Renditions: []ladder.Rendition{
+			{Width: 640, Height: 480, BitratekBps: math.Inf(-1), VMAF: math.NaN(), CRF: 28},
+		},
+	}
+
+	out, err := emitLadderJSON(res, flags, 100.0)
+	if err != nil {
+		t.Fatalf("emitLadderJSON returned error on non-finite payload: %v", err)
+	}
+	if strings.Contains(out, "NaN") || strings.Contains(out, "Infinity") {
+		t.Errorf("emitLadderJSON output still contains bare NaN/Infinity tokens:\n%s", out)
+	}
+	var got ladderWirePayload
+	if err := json.Unmarshal([]byte(out), &got); err != nil {
+		t.Fatalf("emitLadderJSON output is not valid JSON: %v\n%s", err, out)
+	}
+	// Every float field must be finite after sanitisation.
+	for i, p := range got.Cloud {
+		if math.IsNaN(p.BitratekBps) || math.IsInf(p.BitratekBps, 0) ||
+			math.IsNaN(p.VMAF) || math.IsInf(p.VMAF, 0) ||
+			math.IsNaN(p.TargetVMAF) || math.IsInf(p.TargetVMAF, 0) {
+			t.Errorf("Cloud[%d] still has non-finite floats: %+v", i, p)
+		}
+	}
+	for i, p := range got.Hull {
+		if math.IsNaN(p.BitratekBps) || math.IsInf(p.BitratekBps, 0) ||
+			math.IsNaN(p.VMAF) || math.IsInf(p.VMAF, 0) {
+			t.Errorf("Hull[%d] still has non-finite floats: %+v", i, p)
+		}
+	}
+	for i, r := range got.Renditions {
+		if math.IsNaN(r.BitratekBps) || math.IsInf(r.BitratekBps, 0) ||
+			math.IsNaN(r.VMAF) || math.IsInf(r.VMAF, 0) {
+			t.Errorf("Renditions[%d] still has non-finite floats: %+v", i, r)
+		}
+	}
+}

docs/rebase-notes.md

@@ -32,6 +32,43 @@ touched, so upstream syncs cannot collide.
 (`vmaf_init` / `vmaf_read_pictures` / `vmaf_score_pooled` /
 `vmaf_close`) the Go layer wraps is unchanged; we only renamed a
 local `C.VmafContext*` variable inside Go.
+## vmafx-tune-go deep bug audit (2026-05-31, fix/vmafx-tune-go-audit-20260531)
+
+**Files touched:**
+`pkg/report/report.go`, `pkg/report/sanitize_test.go` (new),
+`pkg/bisect/bisect.go`, `pkg/bisect/nan_parse_test.go` (new),
+`pkg/bisect/timeout_test.go` (new),
+`pkg/encoder/encoder.go`, `pkg/encoder/discover.go`,
+`pkg/encoder/discover_test.go`, `pkg/encoder/discover_cache_test.go`
+(new), `pkg/encoder/timeout_test.go` (new),
+`cmd/vmafx-tune/cmd/compare.go`,
+`cmd/vmafx-tune/cmd/ladder.go`,
+`cmd/vmafx-tune/cmd/ladder_nan_test.go` (new),
+`changelog.d/fixed/0979-vmafx-tune-go-deep-bug-audit.md` (new).
+
+**Rebase impact:** Fork-local only.  Every file lives under
+`pkg/{report,bisect,encoder}` or `cmd/vmafx-tune/`, which are 100%
+fork additions (the `vmafx-tune-go` Stage-1 surface from ADR-0705 /
+ADR-0713; no Netflix upstream counterpart exists).  An upstream
+sync will not encounter conflicts on any of these files.
+
+**On-disk surface changes (relevant to in-tree callers):**
+
+- New public helper `report.SanitizeBisectSamples([]bisect.Sample)
+  []any` — exported so the schema-v2 sweep emitter in
+  `cmd/vmafx-tune/cmd.emitSweepJSON` can apply the same nested
+  NaN→null coercion the Python emitter (`_nan_to_none` in
+  `tools/vmaf-tune/src/vmaftune/compare.py`) has used since the
+  RFC-8259 hardening of 2026-05-17.
+- New env-var knobs `VMAFX_TUNE_ENCODE_TIMEOUT` (default `60m`),
+  `VMAFX_TUNE_SCORE_TIMEOUT` (default `30m`),
+  `VMAFX_TUNE_PROBE_TIMEOUT` (default `30s`) for the ffmpeg / vmaf /
+  ffprobe subprocess upper bounds.  Operators can lower these in
+  CI to fail-fast instead of hanging a job.
+- Codec-discovery cache key is now the binary path, not a one-shot
+  `sync.Once`.  Callers that depended on the old "first probe wins
+  forever" shape (none in tree as of this PR) will see a re-probe
+  on binary-path change.
 
 ---