Harden viz: record validation, error handling, and robustness

- Validate Fortran trailing record markers to detect file corruption
- Add ndim else clauses to catch unsupported dimensionality
- Narrow broad except Exception to specific types in MP4 writer
- Exit with code 1 on MP4 generation failure
- Clean stale frames from _frames/ before rendering
- Validate --format argument against supported formats
- Respect log-scale in MP4 auto-range sampling
- Validate slice_axis parameter in render_3d_slice
- Show available timestep range on --step mismatch
- Handle per-step exceptions in PNG rendering loop
- Improve docstrings for ProcessorData, _detect_endianness, render_mp4

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: sbryngelson

toolchain/mfc/viz/reader.py

@@ -26,11 +26,14 @@
 class ProcessorData:
     """Data from a single processor file.
 
-    m, n, p follow the Fortran header convention: x_cb has m+2 elements,
-    data arrays have (m+1) cells per dimension.  The silo reader uses
-    m = len(x_cb) - 1 (= number of cells) which differs by one, but
-    assembly code only uses x_cb lengths and n > 0 / p > 0 for
-    dimensionality, so both conventions work correctly.
+    m, n, p store dimension metadata but their exact semantics differ
+    between readers:
+      - Binary: m = Fortran header value, x_cb has m+2 elements,
+        cell count is m+1.
+      - Silo: m = cell count = len(x_cb) - 1.
+    Assembly code intentionally avoids using m/n/p for array sizing —
+    it derives everything from x_cb/y_cb/z_cb lengths.  If future code
+    needs m directly, this discrepancy must be resolved.
     """
     m: int
     n: int
@@ -53,7 +56,11 @@ class AssembledData:
 
 
 def _detect_endianness(path: str) -> str:
-    """Detect endianness from the first record marker (should be 16 for header)."""
+    """Detect endianness from the first record marker.
+
+    The header record contains 4 int32s (m, n, p, dbvars) = 16 bytes,
+    so the leading Fortran record marker must be 16.
+    """
     with open(path, 'rb') as f:
         raw = f.read(4)
     if len(raw) < 4:
@@ -80,7 +87,15 @@ def _read_record_endian(f, endian: str) -> bytes:
     payload = f.read(rec_len)
     if len(payload) < rec_len:
         raise EOFError("Unexpected end of file reading record payload")
-    f.read(4)  # trailing marker
+    trail = f.read(4)
+    if len(trail) < 4:
+        raise EOFError("Unexpected end of file reading trailing record marker")
+    trail_len = struct.unpack(f'{endian}i', trail)[0]
+    if trail_len != rec_len:
+        raise ValueError(
+            f"Fortran record marker mismatch: leading={rec_len}, trailing={trail_len}. "
+            "File may be corrupted."
+        )
     return payload
 
 
@@ -197,6 +212,9 @@ def discover_format(case_dir: str) -> str:
 
 def discover_timesteps(case_dir: str, fmt: str) -> List[int]:
     """Return sorted list of available timesteps."""
+    if fmt not in ('binary', 'silo'):
+        raise ValueError(f"Unknown format '{fmt}'. Supported: 'binary', 'silo'.")
+
     if fmt == 'binary':
         # Check root/ first (1D), then p0/
         root_dir = os.path.join(case_dir, 'binary', 'root')
@@ -235,7 +253,7 @@ def discover_timesteps(case_dir: str, fmt: str) -> List[int]:
                         pass
             return sorted(steps)
 
-    return []
+    return []  # no timestep files found in expected directories
 
 
 def _discover_processors(case_dir: str, fmt: str) -> List[int]:

toolchain/mfc/viz/renderer.py

@@ -75,6 +75,10 @@ def render_3d_slice(assembled, varname, step, output, slice_axis='z',  # pylint:
     data_3d = assembled.variables[varname]
 
     axis_map = {'x': 0, 'y': 1, 'z': 2}
+    if slice_axis not in axis_map:
+        raise ValueError(
+            f"Invalid slice_axis '{slice_axis}'. Must be one of: 'x', 'y', 'z'."
+        )
     axis_idx = axis_map[slice_axis]
 
     coords = [assembled.x_cc, assembled.y_cc, assembled.z_cc]
@@ -149,6 +153,10 @@ def render_mp4(varname, steps, output, fps=10,  # pylint: disable=too-many-argum
         read_func: Callable(step) -> AssembledData for loading each frame.
         **opts: Rendering options (cmap, vmin, vmax, dpi, log_scale, figsize,
                 slice_axis, slice_index, slice_value).
+
+    Returns:
+        True if the MP4 was successfully written, False on failure
+        (e.g., missing imageio dependency or encoding error).
     """
     if read_func is None:
         raise ValueError("read_func must be provided for MP4 rendering")
@@ -170,12 +178,19 @@ def render_mp4(varname, steps, output, fps=10,  # pylint: disable=too-many-argum
             sample_steps.append(steps[len(steps) // 2])
 
         all_mins, all_maxs = [], []
+        log_scale = opts.get('log_scale', False)
         for s in sample_steps:
             ad = read_func(s)
             d = ad.variables.get(varname)
             if d is not None:
-                all_mins.append(np.nanmin(d))
-                all_maxs.append(np.nanmax(d))
+                if log_scale:
+                    pos = d[d > 0]
+                    if pos.size > 0:
+                        all_mins.append(np.nanmin(pos))
+                        all_maxs.append(np.nanmax(pos))
+                else:
+                    all_mins.append(np.nanmin(d))
+                    all_maxs.append(np.nanmax(d))
 
         if auto_vmin is None and all_mins:
             opts['vmin'] = min(all_mins)
@@ -185,6 +200,11 @@ def render_mp4(varname, steps, output, fps=10,  # pylint: disable=too-many-argum
     # Write frames as images to a temp directory next to the output file
     output_dir = os.path.dirname(os.path.abspath(output))
     viz_dir = os.path.join(output_dir, '_frames')
+    # Clean stale frames from any interrupted previous run
+    if os.path.isdir(viz_dir):
+        for stale in os.listdir(viz_dir):
+            if stale.endswith('.png'):
+                os.remove(os.path.join(viz_dir, stale))
     os.makedirs(viz_dir, exist_ok=True)
 
     try:
@@ -206,6 +226,11 @@ def render_mp4(varname, steps, output, fps=10,  # pylint: disable=too-many-argum
                       varname, step, frame_path, **opts)
         elif assembled.ndim == 3:
             render_3d_slice(assembled, varname, step, frame_path, **opts)
+        else:
+            raise ValueError(
+                f"Unsupported dimensionality ndim={assembled.ndim} for step {step}. "
+                "Expected 1, 2, or 3."
+            )
 
     # Combine frames into MP4 using imageio + imageio-ffmpeg (bundled ffmpeg)
     frame_files = sorted(f for f in os.listdir(viz_dir) if f.endswith('.png'))
@@ -225,7 +250,7 @@ def render_mp4(varname, steps, output, fps=10,  # pylint: disable=too-many-argum
         for fname in frame_files:
             writer.append_data(imageio.imread(os.path.join(viz_dir, fname)))
         success = True
-    except Exception as exc:  # pylint: disable=broad-except
+    except (OSError, ValueError, RuntimeError) as exc:
         print(f"imageio MP4 write failed: {exc}")
     finally:
         if writer is not None:

toolchain/mfc/viz/viz.py

@@ -61,6 +61,10 @@ def viz():  # pylint: disable=too-many-locals,too-many-statements,too-many-branc
     # Auto-detect or use specified format
     fmt_arg = ARG('format')
     if fmt_arg:
+        if fmt_arg not in ('binary', 'silo'):
+            cons.print(f"[bold red]Error:[/bold red] Unknown format '{fmt_arg}'. "
+                       "Supported formats: 'binary', 'silo'.")
+            sys.exit(1)
         fmt = fmt_arg
     else:
         try:
@@ -138,6 +142,9 @@ def viz():  # pylint: disable=too-many-locals,too-many-statements,too-many-branc
     requested_steps = _parse_steps(step_arg, steps)
     if not requested_steps:
         cons.print(f"[bold red]Error:[/bold red] No matching timesteps for --step {step_arg}")
+        if steps:
+            cons.print(f"[bold]Available range:[/bold] {steps[0]} to {steps[-1]} "
+                       f"({len(steps)} timesteps)")
         sys.exit(1)
 
     # Collect rendering options
@@ -206,6 +213,7 @@ def read_step_all_vars(step):
         else:
             cons.print(f"[bold red]Error:[/bold red] Failed to generate {mp4_path}. "
                        "Ensure imageio and imageio-ffmpeg are installed.")
+            sys.exit(1)
         return
 
     # Single or multiple PNG frames
@@ -215,8 +223,15 @@ def read_step_all_vars(step):
     except ImportError:
         step_iter = requested_steps
 
+    failures = []
     for step in step_iter:
-        assembled = read_step(step)
+        try:
+            assembled = read_step(step)
+        except (FileNotFoundError, EOFError, ValueError) as exc:
+            cons.print(f"[yellow]Warning:[/yellow] Skipping step {step}: {exc}")
+            failures.append(step)
+            continue
+
         output_path = os.path.join(output_base, f'{varname}_{step}.png')
 
         if assembled.ndim == 1:
@@ -228,9 +243,18 @@ def read_step_all_vars(step):
                       varname, step, output_path, **render_opts)
         elif assembled.ndim == 3:
             render_3d_slice(assembled, varname, step, output_path, **render_opts)
+        else:
+            cons.print(f"[yellow]Warning:[/yellow] Unsupported ndim={assembled.ndim} "
+                       f"for step {step}, skipping.")
+            failures.append(step)
+            continue
 
         if len(requested_steps) == 1:
             cons.print(f"[bold green]Saved:[/bold green] {output_path}")
 
-    if len(requested_steps) > 1:
-        cons.print(f"[bold green]Saved {len(requested_steps)} frames to:[/bold green] {output_base}/")
+    rendered = len(requested_steps) - len(failures)
+    if failures:
+        cons.print(f"[yellow]Warning:[/yellow] {len(failures)} step(s) failed: "
+                   f"{failures[:10]}{'...' if len(failures) > 10 else ''}")
+    if rendered > 1:
+        cons.print(f"[bold green]Saved {rendered} frames to:[/bold green] {output_base}/")