[Detail Bug] CLI config reads can fail due to non-atomic config file updates (truncated TOML race)

[detail-app]

Detail Bug Report

https://app.detail.dev/org_5f375fe3-a706-4e9a-a6f7-800f2439b3f6/bugs/bug_2e491e1e-0d71-42df-9ca5-ee5a975ac314

Introduced in #131 by @sachiniyer on Mar 6, 2026

Summary

• Context: The config storage module manages reading and writing the CLI's configuration file.
• Bug: load_config() reads the config file without acquiring a file lock, while update_config() truncates and rewrites the file in-place. This creates a race window where readers observe truncated TOML.
• Actual vs. expected: Readers can observe partial writes; writes should be atomic via temp file + rename.
• Impact: Commands fail with "Failed to parse config" during concurrent reads/writes. User must retry.

Code with Bug

// src/config/storage.rs:57-65
pub fn load_config() -> Result<Config> {
    let path = config_path()?;
    if !path.exists() {
        return Ok(Config::default());
    }

    let contents = fs::read_to_string(path)?;  // <-- BUG 🔴 reads while writer may have truncated/partially rewritten file
    toml::from_str(&contents).context("Failed to parse config")
}

// src/config/storage.rs:122-124
(&file).seek(SeekFrom::Start(0))?;
file.set_len(0)?;  // <-- BUG 🔴 truncates in-place, creating a window where readers see invalid/partial TOML
(&file).write_all(new_contents.as_bytes())?;  // <-- BUG 🔴 reader can observe partial write_all output

Explanation

update_config() clears the config file with set_len(0) and then rewrites the new contents. Because load_config() reads the file with no shared lock and no atomic-write strategy, a concurrent read can capture the file while it is empty or mid-write, yielding syntactically invalid TOML (e.g., a truncated key) and causing toml::from_str() to fail.

Evidence: a stress test with 4 reader threads calling load_config() and 1 writer thread calling update_config() produced ~0.0048% failures (4 errors in 83,342 reads). The observed errors include truncated TOML such as:

TOML parse error at line 2, column 8
  |
2 | api_tok
  |        ^
key with no value, expected `=`

#[serde(default)] does not mitigate this because the TOML parser fails before serde deserialization when the file is syntactically invalid.

Codebase Inconsistency

Most commands propagate load_config() errors directly to the user:

// src/lib.rs:37
let config = config::storage::load_config()?;

Only auth login falls back gracefully:

// src/commands/auth.rs:42
let config = storage::load_config().unwrap_or_default();

Recommended Fix

Write config updates atomically (write to a temp file in the same directory, then rename onto the real path). This ensures readers see either the old full file or the new full file, never a partially-written file, and removes the need to rely on advisory locks for read/write safety.

History

This bug was introduced in commit 61d284a. This commit added file locking to prevent concurrent config overwrites, but only added exclusive locks to writers (update_config()) without adding shared locks to readers (load_config()). Advisory locks only work when ALL participants use them, so the locking is ineffective against read-write races. The commit successfully fixed write-write conflicts but left read-write conflicts unaddressed.

[sachiniyer]

Closing — bug is real but the impact is rare (~0.005% per the original report) and a reliable regression test was hard to land on CI. PR #291 attempted a fix but was closed without merging. Can revisit if it surfaces in practice.