fix(session_context): atomic write to eliminate trailing-characters corruption

[moneyacademyKE]

fix(session_context): write context store atomically

Target branch: v0.3.77 (commit 90adbd3). Patch commit: 8a114c4.
Files: src/brain/tools/context.rs (+23/−5). No new deps, no feature flags.

Problem

session_context's save() wrote the per-session store with fs::write(path, content) directly. fs::write truncates on open, but two concurrent save() calls do not serialize. When several session_context set ops fire in one turn (load → mutate → save), their writes interleave at the syscall level: a shorter write landing over a longer one leaves the longer object's tail bytes in place. The file becomes {valid shorter object}{leftover tail} and the next read fails:

Failed to parse context store: trailing characters at line N column 2

Impact

This was the single largest source of tool failures in the instance's feedback ledger — ~57% of all logged tool_failure events over weeks (4,465 identical "trailing characters" errors). Corruption is transient (a later clean write repairs it), so it shows as a high-but-not-permanent rate, with hard on-disk evidence frozen in repair snapshots:

Snapshot	Valid object	File size	Trailing garbage
context_<id>.json.bak.<ts>	7124 B	7372 B	248 B of a prior longer write
context_<id>.json.workbak	42335 B	42336 B	1 trailing }

Root cause

fs::write is not atomic and provides no mutual exclusion across concurrent writers to the same path.

Fix

Write to a uniquely-named temp file in the same directory, then atomically rename it over the target:

• Unique temp name (pid + monotonic counter) → two concurrent saves never race on the temp file itself.
• rename is atomic on the same filesystem → the store is always either the previous complete object or the new one, never a mix.

Out of scope

• Lost-update semantics: atomic rename makes last-writer-wins — two concurrent set calls could still drop one update (no corruption, but a lost change). A per-session Mutex around load→mutate→save would close that; intentionally left out to keep the patch minimal and reviewable. The trigger (dense concurrent session_context bursts) is also addressed operationally via agent discipline.

Testing

• cargo check (see background run).
• Suggested regression test: spawn N concurrent save() calls with varying payload sizes, then assert every load() returns a valid, complete object.

[adolfousier]

Reviewed against a local checkout. Short version: the diagnosis is right, the fix is right, it passes the gates, and I would like a test and a cleanup path before merge.

Verified

The race is real and correctly described. Worked through it independently: fs::write truncates at open, not per write, so two concurrent savers each hold an fd starting at offset 0. If the longer writer completes first and the shorter one then writes over it, the file retains the longer object's tail. That produces exactly the {valid shorter object}{leftover} shape in the PR body. session_context is genuinely reachable concurrently, since tools execute in parallel.

The fix is sound. Unique temp name (pid + atomic counter) in the same directory, then rename. Same filesystem, so the rename is atomic and a reader sees either the previous complete object or the new one. It also turns a latent path.parent() fall-through into a real error, which is a small bonus.

Gates pass on the branch, using the repo's CI commands:

cargo clippy --all-features --lib --bins --tests -- -D warnings   # clean
cargo test --all-features                                          # 5631 passed, 0 failed

Note the PR mentions cargo check. This repo uses clippy with those flags as the gate; cargo check will not catch what CI catches.

The evidence does not reproduce here

Ran the claim against the maintainer instance's ledger:

SELECT COUNT(*) FROM feedback_ledger WHERE event_type='tool_failure';
-- 7295

SELECT COUNT(*) FROM feedback_ledger
WHERE event_type='tool_failure' AND metadata LIKE '%trailing characters%';
-- 0

Zero matches, zero tool_failure rows with a context dimension, no .bak/.workbak files present, and all six context stores on disk parse as valid JSON.

One thing worth double-checking on your side: in this ledger bash accounts for 4115 of 7295 failures, or 56.4%, which is close to the 57% / 4,465 figures quoted. That may be coincidence, but it is worth re-running the query that produced those numbers to confirm the filter is matching what you think it is.

This does not undermine the patch. The race is demonstrable from the code alone, and the snapshot byte counts in the PR body are concrete. It does mean the impact claim is currently unverifiable outside your deployment, which is a good argument for the test below.

Suggested before merge

1. The regression test the PR proposes. It is the thing that turns the impact claim into something reproducible, and it is short. Tests live under src/tests/ in this repo, registered in src/tests/mod.rs:

// N concurrent saves with different payload sizes, then every load must parse.
// Against the old fs::write this fails; against rename it holds.
let saves = (0..32).map(|i| {
    let path = path.clone();
    tokio::spawn(async move {
        let mut store = ContextStore::new("s".into());
        // vary the serialized length so a short write can land over a long one
        store.set("k", &"x".repeat(if i % 2 == 0 { 8_000 } else { 100 }));
        store.save(&path).await
    })
});
futures::future::join_all(saves).await;
assert!(ContextStore::load(&path, "s").await.is_ok());

2. Clean up the temp file when rename fails. If fs::write succeeds and fs::rename fails, the .tmp stays in ~/.opencrabs/agents/session/ indefinitely, in the same directory as the store. Something like:

if let Err(e) = fs::rename(&tmp, path).await {
    let _ = fs::remove_file(&tmp).await;   // best effort, log the original error
    return Err(ToolError::Io(e));
}

3. Worth a comment, not necessarily a change: no fsync before rename. The write is atomic against concurrent writers but not against power loss, where you can end up with a renamed but unflushed file. For a transient context store that is probably an acceptable trade, but it is worth being explicit that "atomic" here means "against racing writers" so the next reader of this code does not assume durability.

Housekeeping

The branch is based on 90adbd38 and will need a rebase onto current main, which has moved.

Thanks for the clear write-up, particularly the byte-level snapshot table. Diagnosing this from parse errors alone is not easy, and the "out of scope" section correctly identifies that last-writer-wins remains after this change.