From fc97b90ce93ae3cf0d99cb8639a42859c1c47013 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 10 Jul 2026 20:29:21 +0500 Subject: [PATCH] fix(push): fail closed on labels.csv read errors in label + sequence preflight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more mirror-checks swallowed a non-EOF csv.Reader.Read error with `continue`, same class as #221: - readLabelColumnValues (CheckLabelDiversity / ReadLabelValues): a mid-read failure returned a PARTIAL class set with Found=true — under-counting classes could false-reject good data or pass a bad file. Now returns an error; the diversity GATE fails closed, while the value PREVIEW (ReadLabelValues) degrades to Found=false. - CheckSequenceRows: a mid-read failure skipped the unread tail, so null/missing sequence ids there never surfaced locally. Now fails closed. CrossCheckLabels and the tabular schema scan already abort on the same error; this brings the whole package to uniform fail-closed behavior. The trigger is I/O, not malformed CSV (LazyQuotes + FieldsPerRecord=-1 tolerate every bad shape). Each scan is split into a reader-based core (labelColumnValuesFrom / sequenceScanFrom) so the branch is exercised with an injected failing reader; parsing is unchanged and the value parity harness validates the relocation. Addresses the two follow-up Bugbot findings on #219. Co-Authored-By: Claude Opus 4.8 --- internal/push/preflight.go | 66 ++++++++++++++++++++++++++------- internal/push/preflight_test.go | 45 ++++++++++++++++++++++ 2 files changed, 98 insertions(+), 13 deletions(-) diff --git a/internal/push/preflight.go b/internal/push/preflight.go index 84649d81..76007615 100644 --- a/internal/push/preflight.go +++ b/internal/push/preflight.go @@ -457,7 +457,13 @@ func TruncateList(items []string, max int) string { // even an empty string is a real class and every distinct trimmed string // counts. The caller derives the two flags from the label's schema type. func CheckLabelDiversity(csvPath, labelColumn string, dropNASentinels, collapseNumeric bool) error { - v := readLabelColumnValues(csvPath, labelColumn, dropNASentinels, collapseNumeric) + v, err := readLabelColumnValues(csvPath, labelColumn, dropNASentinels, collapseNumeric) + if err != nil { + // A mid-read failure leaves a PARTIAL column — trusting its class count + // would false-reject good data (under-counted classes) or pass a bad + // file. Fail closed, like the text preflight (#221) and CrossCheckLabels. + return err + } // Benign-skip when the column is absent (that's CheckLabelColumn's // diagnostic) or an unreadable file (another check's) — both leave Found // false. Two or more classes is diverse enough. @@ -490,7 +496,11 @@ type LabelReadValues struct { // readLabelColumnValues, so the value preview and the diversity verdict cannot // drift from each other. func ReadLabelValues(csvPath, labelColumn string, dropNASentinels, collapseNumeric bool) LabelReadValues { - return readLabelColumnValues(csvPath, labelColumn, dropNASentinels, collapseNumeric) + // The preview path tolerates a mid-read failure as "not found" (the + // diversity GATE, CheckLabelDiversity, is the one that fails closed on it); + // a partial preview simply reports nothing rather than a wrong count. + v, _ := readLabelColumnValues(csvPath, labelColumn, dropNASentinels, collapseNumeric) + return v } // readLabelColumnValues reads csvPath's label column once and returns its @@ -501,15 +511,29 @@ func ReadLabelValues(csvPath, labelColumn string, dropNASentinels, collapseNumer // the ingestor's per-column read). Unlike the previous early-exit diversity // scan, this reads the whole column to build the full class set + row count — // one scan now backs both the diversity verdict and the value-level preview. -func readLabelColumnValues(csvPath, labelColumn string, dropNASentinels, collapseNumeric bool) LabelReadValues { +func readLabelColumnValues(csvPath, labelColumn string, dropNASentinels, collapseNumeric bool) (LabelReadValues, error) { r, closer, err := openCSVReader(csvPath) if err != nil { - return LabelReadValues{} // Found=false: unreadable file is another check's diagnostic + return LabelReadValues{}, nil // Found=false: unreadable file is another check's diagnostic } defer func() { _ = closer.Close() }() + v, err := labelColumnValuesFrom(r, labelColumn, dropNASentinels, collapseNumeric) + if err != nil { + // Mid-read failure: the column is now partial. Fail closed rather than + // count a truncated class set (matches CrossCheckLabels / #221). + return LabelReadValues{}, fmt.Errorf("reading %s: %w", filepath.Base(csvPath), err) + } + return v, nil +} + +// labelColumnValuesFrom is the label scan over an already-opened reader — split +// out so the mid-stream read-error path can be exercised with an injected +// failing reader. A benign miss (no header, or the column absent) returns +// Found=false with a nil error; only a mid-scan read failure is a non-nil error. +func labelColumnValuesFrom(r *csv.Reader, labelColumn string, dropNASentinels, collapseNumeric bool) (LabelReadValues, error) { header, err := r.Read() if err != nil { - return LabelReadValues{} + return LabelReadValues{}, nil // no header (empty/unreadable) — another check's diagnostic } col, resolved := -1, "" for i, c := range header { @@ -528,7 +552,7 @@ func readLabelColumnValues(csvPath, labelColumn string, dropNASentinels, collaps } } if col == -1 { - return LabelReadValues{} // Found=false — benign skip, like the ingestor + return LabelReadValues{}, nil // Found=false — benign skip, like the ingestor } distinct := map[string]bool{} rowCount := 0 @@ -538,7 +562,7 @@ func readLabelColumnValues(csvPath, labelColumn string, dropNASentinels, collaps break } if err != nil { - continue + return LabelReadValues{}, err // caller wraps with the filename and fails closed } rowCount++ if len(rec) <= col { @@ -564,7 +588,7 @@ func readLabelColumnValues(csvPath, labelColumn string, dropNASentinels, collaps classes = append(classes, k) } sort.Strings(classes) - return LabelReadValues{Resolved: resolved, Classes: classes, RowCount: rowCount, Found: true} + return LabelReadValues{Resolved: resolved, Classes: classes, RowCount: rowCount, Found: true}, nil } // knownMediaExtensions mirrors the ingestor's FileExtension.get_all_extensions @@ -731,13 +755,29 @@ func CheckSequenceRows(csvPath, groupColumn string) (sequences int, err error) { return 0, nil // unreadable file is another check's diagnostic } defer func() { _ = closer.Close() }() + seqs, nullErr, readErr := sequenceScanFrom(r, groupColumn) + if readErr != nil { + // Mid-read failure: null/missing ids in the unread tail would never + // surface. Fail closed rather than pass a partial scan (matches + // CrossCheckLabels / #221). + return 0, fmt.Errorf("reading %s: %w", filepath.Base(csvPath), readErr) + } + return seqs, nullErr +} + +// sequenceScanFrom scans the group column over an already-opened reader — split +// out so the mid-stream read-error path can be exercised with an injected +// failing reader. nullErr is the domain rejection (empty/null ids); readErr is +// a mid-scan read failure the caller wraps with the filename. A benign miss (no +// header, or the column absent) returns zero values. +func sequenceScanFrom(r *csv.Reader, groupColumn string) (sequences int, nullErr, readErr error) { header, err := r.Read() if err != nil { - return 0, nil + return 0, nil, nil // no header — benign } col := matchColumnIndex(header, groupColumn) if col == -1 { - return 0, nil // benign skip — the schema checks own this diagnostic + return 0, nil, nil // benign skip — the schema checks own this diagnostic } distinct := map[string]bool{} nullCount, rowNum, firstNullRow := 0, 0, 0 @@ -747,7 +787,7 @@ func CheckSequenceRows(csvPath, groupColumn string) (sequences int, err error) { break } if err != nil { - continue + return 0, nil, err // caller wraps with the filename and fails closed } rowNum++ v := "" @@ -768,9 +808,9 @@ func CheckSequenceRows(csvPath, groupColumn string) (sequences int, err error) { "the sequence column %q has %d empty/null value(s) (first at data row %d). Every "+ "timestep row must carry the id of the sequence it belongs to — the cluster rejects "+ "this after the upload; fill in the ids and re-run.", - groupColumn, nullCount, firstNullRow) + groupColumn, nullCount, firstNullRow), nil } - return len(distinct), nil + return len(distinct), nil, nil } // PreflightProblem is a preflight rejection. BadFlag marks problems whose diff --git a/internal/push/preflight_test.go b/internal/push/preflight_test.go index 9fb5259a..45059954 100644 --- a/internal/push/preflight_test.go +++ b/internal/push/preflight_test.go @@ -1,6 +1,8 @@ package push import ( + "encoding/csv" + "errors" "image" "image/png" "os" @@ -577,3 +579,46 @@ func TestPreflightDataset_SequenceGrouped(t *testing.T) { t.Errorf("time_series_forecasting must stay ungrouped and diversity-free: %v", problem.Err) } } + +// TestLabelColumnValuesFrom_ReadErrorFailsClosed: a mid-scan read error on the +// label column must abort with an error (fail closed) rather than return a +// PARTIAL class set — a truncated count would false-reject good data or pass a +// bad file. Mirrors the text preflight (#221) and CrossCheckLabels. The trigger +// is I/O, not malformed CSV (LazyQuotes + FieldsPerRecord=-1 parse every bad +// shape), so it's exercised with an injected reader that fails after the header. +func TestLabelColumnValuesFrom_ReadErrorFailsClosed(t *testing.T) { + sentinel := errors.New("disk gave up mid-read") + r := csv.NewReader(&failAfterReader{data: []byte("label\n"), err: sentinel}) + r.FieldsPerRecord = -1 // match openCSVReader + + v, err := labelColumnValuesFrom(r, "label", false, false) + if err == nil { + t.Fatal("labelColumnValuesFrom returned nil error on a mid-scan read failure; must fail closed") + } + if !errors.Is(err, sentinel) { + t.Errorf("error should wrap the underlying read failure, got: %v", err) + } + if v.Found { + t.Error("a failed read must not report Found=true (a partial column)") + } +} + +// TestSequenceScanFrom_ReadErrorFailsClosed: the sequence-id scan must surface a +// mid-scan read error (readErr) rather than silently pass a partial scan whose +// unread tail could hide null ids. Same fail-closed contract as #221. +func TestSequenceScanFrom_ReadErrorFailsClosed(t *testing.T) { + sentinel := errors.New("disk gave up mid-read") + r := csv.NewReader(&failAfterReader{data: []byte("sequence_id\n"), err: sentinel}) + r.FieldsPerRecord = -1 // match openCSVReader + + _, nullErr, readErr := sequenceScanFrom(r, "sequence_id") + if readErr == nil { + t.Fatal("sequenceScanFrom returned nil readErr on a mid-scan read failure; must fail closed") + } + if !errors.Is(readErr, sentinel) { + t.Errorf("readErr should wrap the underlying read failure, got: %v", readErr) + } + if nullErr != nil { + t.Errorf("a read failure must not be reported as a null-id domain error: %v", nullErr) + } +}