Skip to content

cli: convert -o output write is non-atomic and panics on IO errors #855

Description

@bpowers

Problem

simlin convert -o <path> writes its converted output (any format: mdl/xmile/json/protobuf) via a bare truncate-and-write with .unwrap():

// src/simlin-cli/src/main.rs:720-722
let output_path = output.unwrap_or_else(|| PathBuf::from("/dev/stdout"));
let mut output_file = File::create(&output_path).unwrap();
output_file.write_all(&buf).unwrap();

Two issues:

  1. Non-atomic: File::create unconditionally truncates the destination. If the write fails mid-way (disk full, interrupted), the user is left with a truncated file at the destination -- potentially destroying the previous good version of the file they were overwriting.
  2. Panics instead of clean errors: any IO failure produces an unwrap() panic with a backtrace rather than the CLI's clean die! message.

This diverges from the repo's atomic-write discipline: simlin_engine::io::atomic_write (tempfile + fsync + rename) already exists in the engine specifically for CLI/MCP/serve use, and simlin-serve routes all its saves through it (src/simlin-serve/src/writer.rs:190-195) for exactly this reason.

Why it matters

Correctness/robustness: conversion errors themselves are caught before the file is created, so the exposure is limited to IO failures mid-write -- but that failure mode silently clobbers the destination file. Also developer experience: a panic backtrace for disk full is poor CLI behavior.

Component

  • src/simlin-cli/src/main.rs (Command::Convert output path, lines ~720-722)

Suggested fix

Route the file output through simlin_engine::io::atomic_write when the target is a regular file path, and keep the plain stream write for the stdout case. Note the current code defaults to PathBuf::from("/dev/stdout") when -o is omitted -- atomic rename cannot work against /dev/stdout (or other non-regular targets like pipes/-), so the stdout default should be handled as a direct io::stdout() write rather than a path. On IO error, print a clean die! message instead of unwrapping.

Severity

Low -- CLI-only, and simlin-cli is primarily a testing/debugging tool.

Context

Identified during a review of write paths after the atomic-write work in simlin-serve.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions