From 406ed30aa7a09cf13dce13c9bcba6e60f623e1ff Mon Sep 17 00:00:00 2001 From: finn763 <165816600+finn763@users.noreply.github.com> Date: Sat, 19 Sep 2026 04:51:37 +0800 Subject: [PATCH] fix(dashboard): a /chat tab's attach token is per tab, and one token maps to one PTY MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two tabs on the dashboard /chat collapsed into one session: ChatPage kept its ?attach= token in localStorage, which every tab on the origin shares, and the PTY registry maps one token to exactly one PTY — so the second tab closed the first one with 4409 and both showed one conversation/model. The token now lives per tab in sessionStorage and is claimed with a Web Lock, so a second tab (including a Chrome "Duplicate tab", which clones sessionStorage) mints its own instead of sharing one; a reload finds its own token free again and still reattaches to the living PTY. PtySessionRegistry.attach_or_spawn also spanned awaits, so two connections racing one token both forked a PTY: the token then mapped to whichever registered last while the other tab's live session fell out of the registry (never reaped, invisible to close_all), and a reattach landed on the other tab's terminal. The get-or-spawn decision is serialized now. Issue #115304. --- hermes_cli/pty_session.py | 41 +++++--- tests/hermes_cli/test_pty_session.py | 42 ++++++++ web/src/lib/pty-attach-token.test.ts | 143 +++++++++++++++++++++++++++ web/src/lib/pty-attach-token.ts | 76 ++++++++++++++ web/src/pages/ChatPage.tsx | 33 +------ 5 files changed, 292 insertions(+), 43 deletions(-) create mode 100644 web/src/lib/pty-attach-token.test.ts create mode 100644 web/src/lib/pty-attach-token.ts diff --git a/hermes_cli/pty_session.py b/hermes_cli/pty_session.py index dd5878f2d8..5c76977154 100644 --- a/hermes_cli/pty_session.py +++ b/hermes_cli/pty_session.py @@ -173,24 +173,35 @@ class PtySessionRegistry: self._buffer_cap = buffer_cap self._read_timeout = read_timeout self._sessions: Dict[str, PtySession] = {} + # The get-or-spawn decision spans awaits (reap_idle, the spawn thread, + # session.start), so two connections racing one attach token both saw + # "no session" and forked a PTY each: the token then mapped to whichever + # registered last while the other tab's live session fell out of the + # registry — never reaped, and a reattach landed on the wrong terminal + # (#115304). Serialize the decision so a token maps to one PTY. + # ponytail: one registry-wide lock, not per key — argv resolution is + # already serialized globally for the same reason, and a spawn only + # delays NEW chats. Per-key locks if spawn throughput ever matters. + self._attach_lock = asyncio.Lock() async def attach_or_spawn(self, key: str, *, spawn: Callable[[], object]) -> Tuple[PtySession, bool]: await self.reap_idle() - existing = self._sessions.get(key) - if existing is not None and existing.alive: - return existing, False - if existing is not None: # dead remnant - await existing.close() - self._sessions.pop(key, None) - if len(self._sessions) >= self._max: - self._reap_one_idle_or_raise() - # PTY spawn does blocking fork/exec work — keep it off the event loop. - # See #53227. - bridge = await asyncio.to_thread(spawn) - session = PtySession(key, bridge, buffer_cap=self._buffer_cap, read_timeout=self._read_timeout) - await session.start() - self._sessions[key] = session - return session, True + async with self._attach_lock: + existing = self._sessions.get(key) + if existing is not None and existing.alive: + return existing, False + if existing is not None: # dead remnant + await existing.close() + self._sessions.pop(key, None) + if len(self._sessions) >= self._max: + self._reap_one_idle_or_raise() + # PTY spawn does blocking fork/exec work — keep it off the event loop. + # See #53227. + bridge = await asyncio.to_thread(spawn) + session = PtySession(key, bridge, buffer_cap=self._buffer_cap, read_timeout=self._read_timeout) + await session.start() + self._sessions[key] = session + return session, True def detach(self, key: str, ws) -> None: s = self._sessions.get(key) diff --git a/tests/hermes_cli/test_pty_session.py b/tests/hermes_cli/test_pty_session.py index cf9e222f60..d8b9ccda94 100644 --- a/tests/hermes_cli/test_pty_session.py +++ b/tests/hermes_cli/test_pty_session.py @@ -324,6 +324,48 @@ async def test_new_key_at_capacity_raises_when_none_reapable(): await reg.close_all() +@pytest.mark.asyncio +async def test_concurrent_attach_on_one_token_forks_one_pty(): + """Two connections racing one attach token must share ONE registered PTY. + + The get-or-spawn decision spans awaits (reap, the spawn thread, start()), so + both racing callers used to see "no session" and fork their own: the token + then mapped to whichever registered last, the other tab's live session fell + out of the registry (never reaped) and a reattach landed on the wrong + terminal (#115304). + """ + from hermes_cli.pty_session import WS_CLOSE_SUPERSEDED + + reg = make_registry() + spawned = [] + + def spawn(): + bridge = FakeBridge([b"", b""]) + spawned.append(bridge) + return bridge + + (s1, created1), (s2, created2) = await asyncio.gather( + reg.attach_or_spawn("tok", spawn=spawn), + reg.attach_or_spawn("tok", spawn=spawn), + ) + + assert len(spawned) == 1 # one token, one PTY + assert (s1, created1) == (s2, True) + assert created2 is False + assert s1.bridge is spawned[0] + assert list(reg._sessions.values()) == [s1] # every handed-out session is tracked + + # Whichever socket attached last owns the terminal; the loser is superseded + # by contract, so no viewer is left writing into an untracked PTY. + ws_a, ws_b = FakeWS(), FakeWS() + await s1.attach(ws_a) + await s2.attach(ws_b) + assert reg._sessions["tok"] is s1 + assert s1._ws is ws_b and ws_b.close_code is None + assert ws_a.close_code == WS_CLOSE_SUPERSEDED + await reg.close_all() + + @pytest.mark.asyncio async def test_reaper_loop_invokes_reap(monkeypatch): from hermes_cli.pty_session import run_reaper diff --git a/web/src/lib/pty-attach-token.test.ts b/web/src/lib/pty-attach-token.test.ts new file mode 100644 index 0000000000..8799afb987 --- /dev/null +++ b/web/src/lib/pty-attach-token.test.ts @@ -0,0 +1,143 @@ +// @vitest-environment jsdom +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * One "browser tab": its own sessionStorage, its own module state (the claimed + * token) and the pages share the fake Web Locks manager, exactly like tabs on + * one origin do. + */ +type Tab = { + ptyAttachToken: (rotate?: boolean) => Promise; + storage: Storage; +}; + +const KEY = "hermes.pty.token.chat"; + +function fakeStorage(seed: Record = {}): Storage { + const store: Record = { ...seed }; + return { + clear: () => { + for (const key of Object.keys(store)) delete store[key]; + }, + getItem: (key: string) => store[key] ?? null, + key: (index: number) => Object.keys(store)[index] ?? null, + get length() { + return Object.keys(store).length; + }, + removeItem: (key: string) => { + delete store[key]; + }, + setItem: (key: string, value: string) => { + store[key] = String(value); + }, + } as Storage; +} + +/** Web Locks, as far as attach-token claiming uses them. */ +function fakeLocks(held: Set, requests: string[] = []) { + return { + request: ( + name: string, + _options: { ifAvailable?: boolean }, + callback: (lock: { name: string } | null) => unknown, + ) => { + requests.push(name); + if (held.has(name)) { + callback(null); + return Promise.resolve(); + } + held.add(name); + return Promise.resolve(callback({ name })); + }, + }; +} + +/** The lock a tab holds disappears when the document unloads (reload/close). */ +function simulateUnload(held: Set) { + held.clear(); +} + +async function openTab( + held: Set, + seed: Record = {}, + requests: string[] = [], +): Promise { + vi.resetModules(); // a new document has its own claimed-token state + const storage = fakeStorage(seed); + Object.defineProperty(window, "sessionStorage", { configurable: true, value: storage }); + Object.defineProperty(window.navigator, "locks", { + configurable: true, + value: fakeLocks(held, requests), + }); + const { ptyAttachToken } = await import("./pty-attach-token"); + return { ptyAttachToken, storage }; +} + +beforeEach(() => { + vi.stubGlobal("crypto", { + getRandomValues: (values: Uint8Array) => { + values.fill(Math.floor(Math.random() * 256)); + return values; + }, + }); +}); + +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + delete (window.navigator as { locks?: unknown }).locks; +}); + +describe("ptyAttachToken", () => { + it("mints its own token for a duplicate tab instead of sharing the live one", async () => { + const held = new Set(); + const first = await openTab(held); + const tokenA = await first.ptyAttachToken(); + + // Chrome's "Duplicate tab" clones sessionStorage into the new tab. + const duplicate = await openTab(held, { [KEY]: tokenA }); + const tokenB = await duplicate.ptyAttachToken(); + + expect(tokenB).not.toBe(tokenA); + expect(duplicate.storage.getItem(KEY)).toBe(tokenB); + }); + + it("reuses the stored token after a reload, once the old document released it", async () => { + const held = new Set(); + const before = await openTab(held, { [KEY]: "tab-token" }); + expect(await before.ptyAttachToken()).toBe("tab-token"); + + simulateUnload(held); // reload: the previous document's lock is gone + const reloaded = await openTab(held, { [KEY]: "tab-token" }); + + expect(await reloaded.ptyAttachToken()).toBe("tab-token"); + }); + + it("claims the token once per document so a reconnect never re-locks it", async () => { + const held = new Set(); + const requests: string[] = []; + const tab = await openTab(held, { [KEY]: "tab-token" }, requests); + + expect(await tab.ptyAttachToken()).toBe("tab-token"); + expect(await tab.ptyAttachToken()).toBe("tab-token"); + + expect(requests).toEqual([`hermes.pty.attach.tab-token`]); + }); + + it("rotates the token for an explicit fresh session", async () => { + const held = new Set(); + const tab = await openTab(held, { [KEY]: "tab-token" }); + + const rotated = await tab.ptyAttachToken(true); + + expect(rotated).not.toBe("tab-token"); + expect(tab.storage.getItem(KEY)).toBe(rotated); + }); + + it("still isolates tabs on sessionStorage when the browser has no Web Locks", async () => { + const tab = await openTab(new Set(), { [KEY]: "tab-token" }); + Object.defineProperty(window.navigator, "locks", { configurable: true, value: undefined }); + + expect(await tab.ptyAttachToken()).toBe("tab-token"); + }); +}); diff --git a/web/src/lib/pty-attach-token.ts b/web/src/lib/pty-attach-token.ts new file mode 100644 index 0000000000..e52db96745 --- /dev/null +++ b/web/src/lib/pty-attach-token.ts @@ -0,0 +1,76 @@ +/** + * Identity of THIS browser tab's keep-alive PTY, sent as `?attach=`. + * + * The dashboard's PTY registry maps one attach token to exactly one PTY, so a + * token two tabs present makes the second tab take the first one's terminal + * over (the first is closed 4409 and goes dead — the reported multi-tab + * interference). localStorage is shared per ORIGIN, so every tab read the same + * value, and Chrome's "Duplicate tab" clones sessionStorage into the new tab, + * so neither storage alone isolates the tabs. The token therefore lives in + * sessionStorage *and* is claimed with a Web Lock: a tab whose token is + * already claimed by a live document mints its own instead of sharing one. + * See #115304. + */ + +const PTY_ATTACH_TOKEN_KEY = "hermes.pty.token.chat"; + +/** The token this document claimed — a reconnect must not re-request the lock + * (Web Locks are not reentrant, so asking twice in one document deadlocks). */ +let claimedToken: string | null = null; + +function tabStorage(): Storage | null { + try { + return typeof window === "undefined" ? null : window.sessionStorage; + } catch { + return null; /* private mode / storage blocked */ + } +} + +function mint(): string { + const a = new Uint8Array(16); + crypto.getRandomValues(a); + return Array.from(a, (b) => b.toString(16).padStart(2, "0")).join(""); +} + +/** Never settles, so the lock stays held for this document's lifetime. */ +const HOLD: Promise = new Promise(() => {}); + +/** + * Claim `token` for this document. True when we now hold it; the browser + * releases it when the document unloads, so a reload of this tab finds the + * token free again while a live second tab does not. Browsers without the Web + * Locks API fall back to sessionStorage isolation only. + */ +async function claim(token: string): Promise { + const locks = typeof navigator === "undefined" ? undefined : navigator.locks; + if (!locks) return true; + return new Promise((resolve) => { + void locks.request(`hermes.pty.attach.${token}`, { ifAvailable: true }, (lock) => { + resolve(lock !== null); + return lock ? HOLD : Promise.resolve(); + }); + }); +} + +/** + * This tab's `?attach=` token, minted when `rotate` starts a fresh session (the + * old keep-alive PTY must NOT be reattached) or when another live tab already + * claimed the stored one. + */ +export async function ptyAttachToken(rotate = false): Promise { + const stored = + claimedToken ?? tabStorage()?.getItem(PTY_ATTACH_TOKEN_KEY) ?? ""; + if (!rotate && stored && (stored === claimedToken || (await claim(stored)))) { + claimedToken = stored; + return stored; + } + const token = mint(); + try { + tabStorage()?.setItem(PTY_ATTACH_TOKEN_KEY, token); + } catch { + /* ignore */ + } + await claim(token); + claimedToken = token; + return token; +} diff --git a/web/src/pages/ChatPage.tsx b/web/src/pages/ChatPage.tsx index aa7f4017ac..5cb6ce777d 100644 --- a/web/src/pages/ChatPage.tsx +++ b/web/src/pages/ChatPage.tsx @@ -92,39 +92,16 @@ import { ptyRejectionBanner, type PtyBannerAction, } from "@/lib/pty-close-copy"; +import { ptyAttachToken } from "@/lib/pty-attach-token"; import { loseWebglContexts } from "@/lib/xterm-webgl-release"; import { PluginSlot } from "@/plugins"; import { useTheme } from "@/themes"; import { useProfileScope } from "@/contexts/useProfileScope"; import { errorMessage } from "@/lib/api-error"; -// Stable per-browser token identifying THIS chat tab's keep-alive PTY session. -// Sent as ?attach=; lets a refresh/disconnect reattach to the same live process -// instead of spawning a fresh one. Per-localStorage, so other devices can't grab it. -// ``rotate`` mints a new token — used when the user explicitly starts a fresh -// session so the old keep-alive PTY is NOT reattached (the registry reaps it). -const PTY_ATTACH_TOKEN_KEY = "hermes.pty.token.chat"; -function ptyAttachToken(rotate = false): string { - let t = ""; - if (!rotate) { - try { - t = window.localStorage.getItem(PTY_ATTACH_TOKEN_KEY) ?? ""; - } catch { - /* private mode / storage blocked */ - } - } - if (!t) { - const a = new Uint8Array(16); - crypto.getRandomValues(a); - t = Array.from(a, (b) => b.toString(16).padStart(2, "0")).join(""); - try { - window.localStorage.setItem(PTY_ATTACH_TOKEN_KEY, t); - } catch { - /* ignore */ - } - } - return t; -} +// Per-tab keep-alive identity (`?attach=`): lives in pty-attach-token.ts so a +// second tab — including a Chrome "Duplicate tab" — gets its own PTY instead of +// taking over this one. See #115304. // Channel id ties this chat tab's PTY child (publisher) to its sidebar // (subscriber). Generated once per mount so a tab refresh starts a fresh @@ -1254,7 +1231,7 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) { // Keep-alive identity: reattach to this tab's living PTY across // refresh/transient drops. A forced-fresh start rotates the token so // the previous keep-alive PTY is not reattached (registry reaps it). - params.attach = ptyAttachToken(forceFresh); + params.attach = await ptyAttachToken(forceFresh); // Profile-scoped chat: the PTY child gets HERMES_HOME pointed at the // selected profile, so the conversation runs with that profile's model, // skills, memory, and sessions (see web_server._resolve_chat_argv).