[Detail Bug] Config writes can lose existing data due to truncation before file lock in save_config

[detail-app]

Detail Bug Report

https://app.detail.dev/org_5f375fe3-a706-4e9a-a6f7-800f2439b3f6/bugs/bug_3b5a0cd5-08da-482d-b222-ce83e5acb944

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

Summary

• Context: The save_config function in src/config/storage.rs:67-75 was updated in commit 61d284a to add file locking, but uses an incorrect pattern (truncate-before-lock).
• Bug: save_config truncates the file BEFORE acquiring the exclusive lock. When racing with update_config, this causes update_config to read an empty file and lose existing config data.
• Actual vs. expected: Actual: concurrent save_config + update_config can result in update_config reading an empty config (due to an unlocked truncate) and writing back a config missing previously persisted fields. Expected: truncation should only occur while holding the exclusive lock so readers/writers never observe an empty file mid-update.
• Impact: Currently no production callers. Test code uses save_config for initialization; function is public so future callers could hit config corruption under concurrency.

Code with Bug

// src/config/storage.rs:67-75
pub fn save_config(config: &Config) -> Result<()> {
    let path = config_path()?;
    let contents = toml::to_string_pretty(config)?;
    let file = File::create(&path)?;  // <-- BUG 🔴 truncates immediately (O_TRUNC) before lock
    file.lock_exclusive()?;           // Lock acquired AFTER truncate
    (&file).write_all(contents.as_bytes())?;
    file.unlock()?;
    Ok(())
}

Explanation

File::create() truncates the target file as part of opening it (O_TRUNC). Because the exclusive lock is only acquired afterwards, there is a race window where the file is empty and unlocked. If another thread concurrently runs update_config, it can acquire the lock, read the empty file, and then write out a config derived from that empty state—dropping fields that existed before the race.

The issue is confirmed by a concurrency repro: 178 corruptions out of 1000 concurrent write operations when save_config races with update_config, and 0 corruptions when update_config races with itself.

Codebase Inconsistency

In the same commit, update_config uses the correct locking pattern by avoiding truncation before the lock:

pub fn update_config(f: impl FnOnce(&mut Config)) -> Result<()> {
    let file = File::options()
        .truncate(false)  // <-- does not truncate before lock
        .open(&path)?;
    file.lock_exclusive()?;  // <-- lock first
    // ...
}

Recommended Fix

Change save_config to open without truncation, lock first, then truncate while holding the lock:

pub fn save_config(config: &Config) -> Result<()> {
    let path = config_path()?;
    let contents = toml::to_string_pretty(config)?;

    let file = File::options()
        .write(true)
        .create(true)
        .truncate(false)
        .open(&path)?;
    file.lock_exclusive()?;
    file.set_len(0)?;
    (&file).write_all(contents.as_bytes())?;
    file.unlock()?;
    Ok(())
}

History

This bug was introduced in commit 61d284a when locking was added to save_config using File::create() (immediate truncation) and the lock was acquired afterwards.