Skip to content

state: stop one cathode instance erasing another's - #13

Open
tdwd wants to merge 2 commits into
mainfrom
session-store-concurrency
Open

tdwd wants to merge 2 commits into
mainfrom
session-store-concurrency

Conversation

@tdwd

@tdwd tdwd commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

The bug you saw

/title named a session and printed its confirmation, then the title never
appeared in ctrl+r — and some sessions were missing from the picker entirely.

Your live store had 47 rows and zero titles, and 7 cathode instances
running. That is the whole story: every state file is shared by every running
instance, and each one loaded the file at its own start. A write that rebuilt
the file from that in-memory snapshot erased everything the other instances had
written since.

A row is only ever written by the instance whose session it is, so the loser of
that race lost the row outright. Touch fires on every turn, so any other
window sending a prompt was enough. Store-only rows (a codex thread) vanished
from the picker; a claude session survived only because listClaudeSessions
re-reads the JSONLs from disk — which is exactly why this read as a display bug
rather than a data-loss one.

Reproduced before fixing:

a, b := openSessionStore(), openSessionStore()   // both start on an empty file
a.Touch("sess-a", ...); a.SetTitle("sess-a", "the one A named")
b.Touch("sess-b", ...)                           // B has never heard of sess-a
// → A's title = "", and sess-a is gone from the file

The fix

The file is the state; the in-memory map is a cache of it.

  • A write is a read-modify-write under flock, never a dump of the cache.
    sessionStore.mutate is the only thing that writes the file.
  • A read re-reads too, or the picker cannot list a session started in
    another window.
  • flock rather than an O_EXCL lockfile, because the kernel releases it
    when the process dies. A lockfile left behind by a killed cathode would wedge
    every other instance, and breaking a stale one needs a timeout nobody can pick
    correctly. Windows has no flock, so it keeps today's behaviour behind a build
    tag — the far narrower race between the read and the rename.
  • The temp file is uniquely named. Two instances sharing one
    sessions.jsonl.tmp wrote into the same file and renamed a half-finished one
    into place, which is the one way the whole store goes at once.

The same bug next door

Prompt history had it at the same frequency. Appends are O_APPEND and safe,
but maxHistoryEntries is 50, so an established history takes the cap-trim path
on nearly every turn — the whole-file rewrite was the write that actually ran,
and the append path was effectively dead. load now owns the cap rule for both
startup and append, and re-reads under the lock.

One deliberate behaviour change falls out: a prompt typed in another window is
now in this window's recall immediately, as it already was after a restart.

readJSONL/writeJSONL are shared (statefile.go), because the two stores had
each grown their own and only one of them had a unique temp name.

settings.go still has the shape and is recorded in CLAUDE.md rather than
fixed: saveSettings writes the whole struct with os.WriteFile, so a theme
change in one window reverts another window's diff-style change, and a crash
mid-write truncates the file. A read-modify-write cannot fix it alone — it
cannot tell a stale field from a deliberate one without the call sites saying
which field changed. Say if you want that next.

Tests

Four new tests, and each of the four mechanisms was confirmed load-bearing by
reverting it and watching the right test fail:

reverted fails
re-read in mutate TestAWriteKeepsWhatAnotherInstanceWrote
re-read in All() TestThePickerSeesASessionAnotherInstanceStarted
the file lock TestConcurrentWritesKeepEveryRow (5/5 runs)
re-read in history load TestHistoryKeepsWhatAnotherInstanceAppended

go build, go vet and the suite are green, and the package builds and vets
clean for GOOS=windows and GOOS=linux as well.

What this does not do

The titles already lost are not recoverable — they were overwritten. And the
running instances keep their old image, so they will go on clobbering until they
are restarted.

Session titles were lost and sessions went missing from the ctrl+r picker.
/title wrote the store and printed its confirmation, then the next turn in any
other window put the file back to a state that had never seen that row.

Every state file is shared by every running cathode instance, and each one
loaded the file at its own start. A write that rebuilt the file from that
in-memory snapshot erased everything the other instances had written since. A
row is only ever written by the instance whose session it is, so the loser of
the race lost the row outright. Store-only rows (a codex thread) vanished from
the picker; a claude session survived only because its JSONL is re-read from
disk, which is why this read as a display bug.

A write is now a read-modify-write under flock, and a read re-reads too, or the
picker cannot list a session started in another window. flock rather than an
O_EXCL lockfile because the kernel releases it when the process dies. The temp
file is uniquely named: two instances sharing one sessions.jsonl.tmp wrote into
the same file and renamed a half-finished one into place.

Prompt history had the same bug at the same frequency. Appends are O_APPEND and
safe, but maxHistoryEntries is small enough that an established history takes
the cap-trim path on nearly every turn, so the whole-file rewrite was the write
that ran. The trim now re-reads under the lock, and one function owns the cap
rule for both startup and append.

readJSONL and writeJSONL are shared, because the two stores had grown their own
and only one had a unique temp name. settings.go still has the shape, recorded
but not fixed: saveSettings writes the whole struct, so a read-modify-write
cannot tell a stale field from a deliberate one without the call sites saying
which one changed.
…e real files

Two remaining instances of the shape the previous commit fixed.

settings.json had it, and the fix is the interesting one: a read-modify-write
alone cannot do it, because re-reading tells you what is on disk but not which
of the caller's fields is a deliberate change and which is a stale copy. So
saveSettings takes a mutator rather than a settings value — the caller names the
field it is changing, and the signature no longer accepts a whole struct.
model.commitSetting applies the same mutator to the model and to the file and is
the single owner of a settings write; there is no longer a direct m.settings.X
assignment anywhere. A theme commits its banner in the same mutator, or the file
can end up holding one theme with the other's banner. The running UI
deliberately does not re-read: a theme another window picked must not restyle
this one mid-session.

And the test suite reached the developer's own state dir. Any test that builds a
model opens the session store and the prompt history, and most never redirected
XDG_STATE_HOME — so `go test` read those files, and because an established
prompt history sits at its cap, the cap self-heal rewrote the real
prompt-history.jsonl. A package TestMain now points every test at a throwaway
dir, which is what makes it unreachable: a new test cannot touch them by
forgetting a t.Setenv.

replaceFile is now the only way any state file is replaced, so the atomic write
cannot be right in one store and wrong in another. It gained a Sync before the
rename, which is load-bearing rather than hygiene: without it the rename can
reach the disk while the bytes have not, leaving a file that is present and
empty — the one crash the temp-and-rename is there to rule out.

settings.go was 325 lines, so the menu rows and their commit functions moved to
settingsmenu.go, leaving settings.go as the persisted struct and its file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant