From d79a467322094f24e7204c5136ffeef5157ace59 Mon Sep 17 00:00:00 2001 From: rareboe Date: Wed, 26 Aug 2026 10:29:29 +0900 Subject: [PATCH] fix(tui): stop stale instances from erasing pinned sessions Every TUI instance read the global session.json pin list once at startup and kept a private copy. Pinning, unpinning and session.deleted all rewrote the whole file from that copy with an unserialized, fire-and-forget write, so an instance holding a stale snapshot replaced pins added by another instance. Two writers starting from an empty list, one pinning "a" and one pinning "b", end up with just ["b"]. Apply each change as an intent against the file's current contents inside Flock.withLock, the same lock context/kv.tsx already uses, and chain writes off the initial read so they cannot run before it completes. The in-memory update still happens immediately so the UI does not wait on the write, and the store is reconciled with the merged result once it lands. --- packages/tui/src/context/local.tsx | 92 ++++++++++++++----------- packages/tui/test/context/local.test.ts | 57 ++++++++++++++- 2 files changed, 108 insertions(+), 41 deletions(-) diff --git a/packages/tui/src/context/local.tsx b/packages/tui/src/context/local.tsx index a05d315166ed..cfd0d638724e 100644 --- a/packages/tui/src/context/local.tsx +++ b/packages/tui/src/context/local.tsx @@ -9,6 +9,8 @@ import { useArgs } from "./args" import { useSDK } from "./sdk" import { RGBA } from "@opentui/core" import { readJson, writeJsonAtomic } from "../util/persistence" +import { Flock } from "@opencode-ai/core/util/flock" +import { Global } from "@opencode-ai/core/global" import { useTheme } from "./theme" import { useToast } from "../ui/toast" import { useRoute } from "./route" @@ -48,6 +50,34 @@ export function recentModels( .map((item) => ({ providerID: item.providerID, modelID: item.modelID })) } +export function parsePinned(value: unknown) { + if (!value || typeof value !== "object") return [] + const pinned = (value as Record).pinned + if (!Array.isArray(pinned)) return [] + return pinned.filter((item): item is string => typeof item === "string") +} + +export function setPinned(pinned: string[], sessionID: string, pin: boolean) { + if (!pin) return pinned.filter((x) => x !== sessionID) + return pinned.includes(sessionID) ? pinned : [...pinned, sessionID] +} + +// Every TUI instance writes this same file, so a change has to be applied to the file's +// current contents under a lock. Writing the caller's snapshot instead would drop pins +// made by another instance since that snapshot was loaded. +export function persistPinned(filePath: string, sessionID: string, pin: boolean, options?: Flock.Options) { + return Flock.withLock( + `tui-session:${filePath}`, + async () => { + const current = await readJson(filePath).catch(() => undefined) + const next = setPinned(parsePinned(current), sessionID, pin) + await writeJsonAtomic(filePath, { pinned: next }) + return next + }, + options, + ) +} + export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ name: "Local", init: () => { @@ -417,53 +447,42 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ pinned: [], }) + // Touching Global here guarantees Flock's lock root is configured, same as the KV store. + void Global.Path.state const filePath = path.join(paths.state, "session.json") - const state = { - pending: false, - } - - function save() { - if (!sessionStore.ready) { - state.pending = true - return - } - state.pending = false - void writeJsonAtomic(filePath, { - pinned: sessionStore.pinned, - }) - } - readJson(filePath) + // Writes chain off the initial read so they can never run before it completes. + let write = Flock.withLock(`tui-session:${filePath}`, () => readJson(filePath)) .then((x) => { - if (!x || typeof x !== "object") return - const pinned = (x as Record).pinned - if (Array.isArray(pinned)) - setSessionStore( - "pinned", - pinned.filter((item): item is string => typeof item === "string"), - ) + setSessionStore("pinned", parsePinned(x)) }) .catch(() => {}) .finally(() => { setSessionStore("ready", true) - if (state.pending) save() }) + function update(sessionID: string, pin: boolean) { + // Reflect the change locally right away, then reconcile with whatever the merged + // file actually ends up containing. + setSessionStore("pinned", setPinned(sessionStore.pinned, sessionID, pin)) + write = write + .then(() => persistPinned(filePath, sessionID, pin)) + .then((next) => { + setSessionStore("pinned", next) + }) + .catch((error) => { + console.error("Failed to write session state", { error }) + }) + } + const slots = createMemo(() => { const existing = new Set(sync.data.session.filter((x) => x.parentID === undefined).map((x) => x.id)) return sessionStore.pinned.filter((id) => existing.has(id)).slice(0, 9) }) function prune(sessionID: string) { - batch(() => { - if (sessionStore.pinned.includes(sessionID)) { - setSessionStore( - "pinned", - sessionStore.pinned.filter((x) => x !== sessionID), - ) - } - save() - }) + if (!sessionStore.pinned.includes(sessionID)) return + update(sessionID, false) } event.on("session.deleted", (evt) => { @@ -482,14 +501,7 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ return sessionStore.pinned.includes(sessionID) }, togglePin(sessionID: string) { - batch(() => { - const exists = sessionStore.pinned.includes(sessionID) - const next = exists - ? sessionStore.pinned.filter((x) => x !== sessionID) - : [...sessionStore.pinned, sessionID] - setSessionStore("pinned", next) - save() - }) + update(sessionID, !sessionStore.pinned.includes(sessionID)) }, quickSwitch(slot: number) { const target = slots()[slot - 1] diff --git a/packages/tui/test/context/local.test.ts b/packages/tui/test/context/local.test.ts index e2f1e45f75a9..1f9a41e619e6 100644 --- a/packages/tui/test/context/local.test.ts +++ b/packages/tui/test/context/local.test.ts @@ -1,5 +1,9 @@ import { expect, test } from "bun:test" -import { parseModel, recentModels } from "../../src/context/local" +import { mkdtemp } from "fs/promises" +import { tmpdir } from "os" +import path from "path" +import { parseModel, parsePinned, persistPinned, recentModels, setPinned } from "../../src/context/local" +import { readJson } from "../../src/util/persistence" test("parses model IDs containing slashes", () => { expect(parseModel("provider/family/model")).toEqual({ @@ -20,3 +24,54 @@ test("moves a model to the front, deduplicates, and limits recents", () => { ...recent.slice(6, 10), ]) }) + +test("parses only string entries out of a stored pin list", () => { + expect(parsePinned({ pinned: ["a", 1, null, "b"] })).toEqual(["a", "b"]) + expect(parsePinned({ pinned: "a" })).toEqual([]) + expect(parsePinned(undefined)).toEqual([]) +}) + +test("pins and unpins without duplicating entries", () => { + expect(setPinned(["a"], "b", true)).toEqual(["a", "b"]) + expect(setPinned(["a", "b"], "b", true)).toEqual(["a", "b"]) + expect(setPinned(["a", "b"], "a", false)).toEqual(["b"]) + expect(setPinned(["a"], "b", false)).toEqual(["a"]) +}) + +async function pinDir() { + const dir = await mkdtemp(path.join(tmpdir(), "opencode-pins-")) + return { file: path.join(dir, "session.json"), options: { dir: path.join(dir, "locks") } } +} + +test("a pin written by another instance survives a later write", async () => { + const { file, options } = await pinDir() + + await persistPinned(file, "a", true, options) + // Another TUI pins "b" while this instance still believes the list is just ["a"]. + await persistPinned(file, "b", true, options) + + expect(await persistPinned(file, "c", true, options)).toEqual(["a", "b", "c"]) + expect(parsePinned(await readJson(file))).toEqual(["a", "b", "c"]) +}) + +test("concurrent pin writes do not erase each other", async () => { + const { file, options } = await pinDir() + + await Promise.all([ + persistPinned(file, "a", true, options), + persistPinned(file, "b", true, options), + persistPinned(file, "c", true, options), + ]) + + expect(parsePinned(await readJson(file)).sort()).toEqual(["a", "b", "c"]) +}) + +test("unpinning removes only the target session", async () => { + const { file, options } = await pinDir() + + await persistPinned(file, "a", true, options) + await persistPinned(file, "b", true, options) + + expect(await persistPinned(file, "a", false, options)).toEqual(["b"]) + expect(parsePinned(await readJson(file))).toEqual(["b"]) +})