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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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