fix(validate): handle a genuinely missing conflated.parquet cleanly (#766)

#722's own verification checklist asked to "confirm validate correctly
fails on a deliberately-bad case (e.g. an artificially tiny --mem-limit
that should trip the OOM check)" -- never actually done until now.
Ran it for real: a 32m --mem-limit against v0.8.2 on Hetzner, which
OOM-killed the container (confirmed twice in dmesg) before it ever got
past import_atp, so no conflated.parquet was ever uploaded.

That crashed `validate` outright: check_nonempty (and check_match_rate
in the advisory checks) call read_parquet() unconditionally, and
DuckDB raises duckdb.IOException (an HTTP 404 against the S3 test
bucket) rather than returning an empty result for a URL with nothing
at it -- an unhandled traceback instead of a clean failure report,
even though the underlying script still exited non-zero either way.

Fixes both:
- check_nonempty now catches duckdb.IOException and reports a clean
  failed CheckResult instead of propagating the exception.
- run_hard_checks uses that result to skip (not attempt) the five
  other hard checks that also read the same file -- each would fail
  with an identical, redundant "could not read" result for the exact
  same reason. The four log/dmesg-based checks after those don't
  depend on the parquet file at all, so they still run -- and are
  exactly what explains *why* there's no output (here: check_no_oom,
  which correctly reported the real kill).
- check_match_rate (advisory) gets the same try/except, reporting
  "skipped" -- same semantic as its existing regional-extract skip
  right above it, not a failure (advisory checks never fail validate).

Verified against the real broken run's downloaded logs: validate now
prints a clean [FAIL]/[SKIP] report (non-empty output, run completed,
cgroup signal, no OOM, and ATP geometry floor correctly FAIL; the five
parquet-dependent checks correctly SKIP; match rate correctly SKIPs
too) and exits 1, instead of crashing with a traceback.

Verified: uv run pytest from scripts/ (81 passed, 3 new: missing-output
cases for check_nonempty, run_hard_checks, and check_match_rate).

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

Author: brawer

scripts/test-on-hetzner/tests/test_validate.py

@@ -123,6 +123,19 @@ def test_check_nonempty(tmp_path, select_suffix, expected):
     assert v.check_nonempty(con, url).passed is expected
 
 
+def test_check_nonempty_reports_clean_failure_when_output_is_missing(tmp_path):
+    # A run that crashed/was OOM-killed before conflate ever finished
+    # never uploads conflated.parquet at all -- confirmed against a real
+    # Hetzner run during #722's own verification (a deliberately tiny
+    # --mem-limit). Without the try/except in check_nonempty, DuckDB's
+    # duckdb.IOException would propagate straight out of this call as an
+    # unhandled traceback instead of a reportable CheckResult.
+    con = duckdb.connect()
+    result = v.check_nonempty(con, str(tmp_path / "does-not-exist.parquet"))
+    assert result.passed is False
+    assert "could not read" in result.message
+
+
 @pytest.mark.parametrize(
     "sql,expected",
     [
@@ -323,6 +336,40 @@ def test_run_hard_checks_returns_all_checks_in_order(tmp_path, monkeypatch):
     assert all(r.passed for r in results)
 
 
+def test_run_hard_checks_skips_parquet_checks_when_output_is_missing(tmp_path):
+    # Mirrors a real crashed/OOM-killed run: no conflated.parquet was
+    # ever uploaded. The five checks that also read that file should be
+    # skipped (not attempted and failed one by one for the same
+    # underlying reason) -- but the log/dmesg-based checks don't need
+    # the file at all, and should still run and report the real signal
+    # (here: the OOM that's exactly why there's no output).
+    con = duckdb.connect()
+    url = str(tmp_path / "does-not-exist.parquet")
+    records = [
+        {"level": "INFO", "message": "conflate: start", "fields": {"step": "conflate", "phase": "start"}},
+    ]
+    dmesg_text = "Memory cgroup out of memory: Killed process 2699 (osm-diffs)"
+    results = v.run_hard_checks(
+        con, url, records, dmesg_text=dmesg_text, mem_limit="4g", expect_pipeline_version=None, min_atp_features=None
+    )
+    by_name = {r.name: r for r in results}
+
+    assert by_name["non-empty output"].passed is False
+    for name in [
+        "schema",
+        "atp/atp_geometry null-consistency",
+        "osm/osm_geometry null-consistency",
+        "osm_geometry validity",
+        "provenance BOM",
+    ]:
+        assert by_name[name].passed is None, f"{name} should be skipped, not attempted"
+
+    # Not parquet-dependent -- still actually run, and still report the
+    # real failure (the OOM), not silently skipped alongside the rest.
+    assert by_name["no OOM"].passed is False
+    assert "killed process" in by_name["no OOM"].message.lower()
+
+
 # ── advisory checks ─────────────────────────────────────────────────
 #
 # Leaner than the hard-check tests above: one test per piece of actual
@@ -338,6 +385,13 @@ def test_check_match_rate_skips_for_regional_extract(tmp_path):
     assert "skipped" in result.message
 
 
+def test_check_match_rate_skips_when_output_is_missing(tmp_path):
+    con = duckdb.connect()
+    result = v.check_match_rate(con, str(tmp_path / "does-not-exist.parquet"), regional_extract=None)
+    assert result.passed is None
+    assert "skipped" in result.message
+
+
 def test_check_match_rate_computes_ratio(tmp_path):
     # One matched row (VALID_ROW_SQL) plus one ATP-only (unmatched) row,
     # built by nulling out osm/osm_geometry on a second copy of it.

scripts/test-on-hetzner/validate.py

@@ -143,7 +143,21 @@ def list_element_type(duckdb_type):
 
 
 def check_nonempty(con, url):
-    count = con.execute("SELECT count(*) FROM read_parquet(?)", [url]).fetchone()[0]
+    # A run that crashed (or was OOM-killed -- see check_no_oom) before
+    # conflate ever finished never uploads a conflated.parquet at all,
+    # so `url` isn't just empty, it's unreadable -- confirmed the hard
+    # way (#722's own verification checklist item, "confirm validate
+    # correctly fails on a deliberately-bad case": an artificially tiny
+    # --mem-limit produced exactly this). Without this try/except,
+    # DuckDB raises duckdb.IOException (an HTTP 404 against the S3
+    # test bucket) straight out of this function, and every other hard
+    # check that also reads `url` would do the same -- an unhandled
+    # traceback instead of a clean failure report, even though the
+    # underlying `validate` script still exits non-zero either way.
+    try:
+        count = con.execute("SELECT count(*) FROM read_parquet(?)", [url]).fetchone()[0]
+    except duckdb.IOException as e:
+        return CheckResult("non-empty output", False, f"could not read {url}: {e}")
     return CheckResult("non-empty output", count > 0, f"{count} rows")
 
 
@@ -350,11 +364,22 @@ def check_match_rate(con, url, regional_extract):
         # AllThePlaces is always worldwide -- a regional OSM extract
         # shows ~0% match outside its region by design, not by defect.
         return CheckResult("match rate", None, "skipped (regional extract)")
-    total, matched = con.execute(
-        "SELECT count(*), count(*) FILTER (WHERE atp IS NOT NULL AND osm IS NOT NULL) "
-        "FROM read_parquet(?)",
-        [url],
-    ).fetchone()
+    # Same missing-output case check_nonempty's own doc comment
+    # explains (a crashed/OOM-killed run never uploads a
+    # conflated.parquet at all) -- there's genuinely no match rate to
+    # report here, same as the regional-extract case just above, so
+    # this is a skip too, not a failure (advisory checks never fail
+    # validate regardless -- see the module docstring -- but without
+    # this, DuckDB's duckdb.IOException would still surface as an
+    # unhandled traceback rather than a clean skip).
+    try:
+        total, matched = con.execute(
+            "SELECT count(*), count(*) FILTER (WHERE atp IS NOT NULL AND osm IS NOT NULL) "
+            "FROM read_parquet(?)",
+            [url],
+        ).fetchone()
+    except duckdb.IOException as e:
+        return CheckResult("match rate", None, f"skipped: could not read {url}: {e}")
     rate = matched / total if total else 0.0
     return CheckResult("match rate", True, f"{matched}/{total} rows matched ({rate:.1%})")
 
@@ -458,14 +483,37 @@ def run_hard_checks(con, url, records, dmesg_text, mem_limit, expect_pipeline_ve
     """Runs every hard check and returns the list of `CheckResult`s, in
     the order printed. Doesn't short-circuit on the first failure --
     `cmd_validate` wants the full picture in one pass, not a
-    fix-one-rerun-find-the-next loop."""
+    fix-one-rerun-find-the-next loop.
+
+    One exception: if `check_nonempty` itself can't even read `url`
+    (see its own doc comment), the five checks below it that also read
+    that same file are skipped rather than attempted -- each would fail
+    with an identical, redundant "could not read" result for the exact
+    same reason. The four log/dmesg-based checks after those don't
+    depend on the parquet file at all, so they still run -- and are
+    exactly what can explain *why* there's no output (e.g. an
+    OOM-kill, via check_no_oom)."""
+    nonempty = check_nonempty(con, url)
+    if not nonempty.passed:
+        skip = 'skipped: no usable output (see "non-empty output" above)'
+        parquet_checks = [
+            CheckResult("schema", None, skip),
+            CheckResult("atp/atp_geometry null-consistency", None, skip),
+            CheckResult("osm/osm_geometry null-consistency", None, skip),
+            CheckResult("osm_geometry validity", None, skip),
+            CheckResult("provenance BOM", None, skip),
+        ]
+    else:
+        parquet_checks = [
+            check_schema(con, url),
+            check_null_consistency(con, url, "atp", "atp_geometry"),
+            check_null_consistency(con, url, "osm", "osm_geometry"),
+            check_geometry_validity(con, url),
+            check_provenance_bom(con, url, expect_pipeline_version),
+        ]
     return [
-        check_nonempty(con, url),
-        check_schema(con, url),
-        check_null_consistency(con, url, "atp", "atp_geometry"),
-        check_null_consistency(con, url, "osm", "osm_geometry"),
-        check_geometry_validity(con, url),
-        check_provenance_bom(con, url, expect_pipeline_version),
+        nonempty,
+        *parquet_checks,
         check_run_completed(records),
         check_cgroup_signal(records, mem_limit),
         check_no_oom(dmesg_text),