From 1c79b298f2abc8c53aff24c7edf3ef91d0460e04 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sun, 3 May 2026 16:42:31 -0400 Subject: [PATCH] fix(pty): derive deterministic sessionId from (worktreeId, tabId, leafId) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: Issue #217 loses terminal scrollback across unclean shutdowns because the minted sessionId is random — the persist-sessionId write races SIGKILL, so the post-restart PTY maps to a fresh history dir instead of the previous one. mintPtySessionId now hashes (tabId, leafId) when both are present so a crash before persistence still lands on the same history dir after restart. Falls back to randomUUID() when callers don't thread pane identity (non-daemon-host spawns and legacy paths). The origin is surfaced via mintPtySessionIdWithOrigin so ipc/pty can log random-fallback at warn — a daemon-host spawn that hits the fallback indicates a plumbing regression in the renderer. Thread tabId/leafId from renderer through PtyTransport → preload → pty:spawn IPC → daemon-pty-adapter fallback so all daemon-host spawns produce deterministic ids. See DESIGN_DOC_TERMINAL_HISTORY_FIX.md §Deterministic sessionId. Co-authored-by: Orca --- src/main/daemon/daemon-pty-adapter.ts | 9 +- src/main/daemon/pty-session-id.ts | 100 ++++++++++++++++-- src/main/ipc/pty.ts | 54 +++++++++- src/main/providers/types.ts | 9 ++ src/preload/api-types.ts | 6 ++ src/preload/index.ts | 2 + .../terminal-pane/pty-connection.ts | 19 ++++ .../terminal-pane/pty-dispatcher.ts | 7 ++ .../components/terminal-pane/pty-transport.ts | 7 +- 9 files changed, 201 insertions(+), 12 deletions(-) diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index 349ca4d5136..cd73232acf7 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -96,7 +96,14 @@ export class DaemonPtyAdapter implements IPtyProvider { private async doSpawn(opts: PtySpawnOptions): Promise { await this.ensureConnected() - const sessionId = opts.sessionId ?? mintPtySessionId(opts.worktreeId) + // Why: the renderer should thread tabId/leafId for every daemon-host + // spawn so we get a deterministic sessionId. On the main-process + // spawn path, `ipc/pty.ts` has already minted `opts.sessionId` with + // the deterministic suffix — we arrive here via opts.sessionId in + // the common case. This fallback handles direct callers (tests, + // non-IPC code paths) that don't pre-mint; they should still pass + // tabId/leafId when possible so the id is stable across restarts. + const sessionId = opts.sessionId ?? mintPtySessionId(opts.worktreeId, opts.tabId, opts.leafId) if (this.killedSessionTombstones.has(sessionId)) { throw new TerminalKilledError(sessionId) diff --git a/src/main/daemon/pty-session-id.ts b/src/main/daemon/pty-session-id.ts index e1088341d44..4d0a2feb04a 100644 --- a/src/main/daemon/pty-session-id.ts +++ b/src/main/daemon/pty-session-id.ts @@ -1,18 +1,104 @@ -import { randomUUID } from 'crypto' +import { createHash, randomUUID } from 'crypto' import { isAbsolute, join, relative, resolve, sep } from 'path' +export type MintedSessionIdOrigin = 'deterministic' | 'explicit' | 'random-fallback' + +export type MintSessionIdResult = { + sessionId: string + /** + * How the id was produced: + * - `deterministic`: derived from (worktreeId, tabId, leafId) — the happy + * path that lets cold-restore survive a crash before persistence. + * - `explicit`: caller supplied a pre-computed sessionId (reattach path). + * - `random-fallback`: a daemon-host spawn did not receive tabId/leafId + * and had to fall back to a random UUID. Persistence still matters for + * this branch — it is the old behavior and re-exposes the race this + * design is trying to close. Logged at warn level so regressions in + * the renderer plumbing surface loudly. + */ + derivedFrom: MintedSessionIdOrigin +} + /** - * Session IDs use the format `${worktreeId}@@${shortUuid}` so that + * Session IDs use the format `${worktreeId}@@${suffix}` so that * DaemonPtyAdapter.reconcileOnStartup (see daemon-pty-adapter.ts) can * derive the owning worktree by splitting on the @@ separator. * - * Both pty.ts (host-daemon spawn path) and DaemonPtyAdapter.doSpawn - * (fallback when opts.sessionId is absent) must use this helper — a - * drifted format would break cold-restore mapping and Pi overlay + * Deterministic suffix (preferred): when both `tabId` and `leafId` are + * provided, the suffix is the first 32 bits of SHA-256(tabId || "\0" || + * leafId), hex-encoded. This lets a restart reach the same history + * directory for the same (worktreeId, tabId, leafId) triple *without* + * needing `orca-data.json` to have persisted the sessionId — closing + * the SIGKILL-between-spawn-and-persist race that Issue #217 describes. + * + * Random suffix (fallback): when tabId/leafId are absent (e.g. an older + * non-pane caller) the suffix is `randomUUID().slice(0, 8)`. This is the + * legacy behavior; the renderer is expected to thread tabId+leafId for + * every daemon-host spawn, so falling in here from a daemon-host code + * path indicates a plumbing bug and is logged at warn level by callers. + * + * Both `pty.ts` (host-daemon spawn path) and DaemonPtyAdapter.doSpawn + * (fallback when opts.sessionId is absent) delegate to this helper — + * a drifted format would break cold-restore mapping and Pi overlay * keying. + * + * Collision bound: with 32 bits of suffix entropy, the birthday bound + * for two distinct (tabId, leafId) pairs colliding within one worktree + * is ~1 in 2^16 at ~256 pairs. Real worktrees hold O(10) panes. The + * bound matches the pre-existing random-UUID path (also 32 bits) — no + * regression. */ -export function mintPtySessionId(worktreeId?: string): string { - return worktreeId ? `${worktreeId}@@${randomUUID().slice(0, 8)}` : randomUUID() +export function mintPtySessionId(worktreeId?: string, tabId?: string, leafId?: string): string { + if (!worktreeId) { + return randomUUID() + } + if (tabId && leafId) { + // Why: NUL separator is belt-and-suspenders. `createHash.update()` is + // streaming so `update("a") + update("\0") + update("b")` and + // `update("a\0b")` produce identical digests; the explicit separator + // is cosmetic against today's NUL-free input alphabet (tabIds are + // `crypto.randomUUID()`, leafIds are `pane:`). Callers that + // might introduce NUL-containing inputs must switch to a + // length-prefixed encoding instead. + const h = createHash('sha256') + h.update(tabId) + h.update('\0') + h.update(leafId) + const suffix = h.digest('hex').slice(0, 8) + return `${worktreeId}@@${suffix}` + } + return `${worktreeId}@@${randomUUID().slice(0, 8)}` +} + +/** + * Variant that also reports how the id was produced. Use this at the + * single mint call site in `ipc/pty.ts` so the observability log can + * split info vs. warn appropriately. + */ +export function mintPtySessionIdWithOrigin(args: { + worktreeId?: string + tabId?: string + leafId?: string + explicitSessionId?: string +}): MintSessionIdResult { + if (args.explicitSessionId) { + return { sessionId: args.explicitSessionId, derivedFrom: 'explicit' } + } + if (args.worktreeId && args.tabId && args.leafId) { + return { + sessionId: mintPtySessionId(args.worktreeId, args.tabId, args.leafId), + derivedFrom: 'deterministic' + } + } + // Either worktreeId is absent (non-daemon spawn — we return a raw uuid + // and the caller is responsible for deciding whether that's expected) + // or tabId/leafId weren't threaded through from the renderer. The + // second case is the one we want to hear about; callers differentiate + // on `!!args.worktreeId` before choosing the log level. + return { + sessionId: mintPtySessionId(args.worktreeId, args.tabId, args.leafId), + derivedFrom: 'random-fallback' + } } /** diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index b8962fc823d..75fa98e30b4 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -15,7 +15,7 @@ import { piTitlebarExtensionService } from '../pi/titlebar-extension-service' import { isPwshAvailable } from '../pwsh' import { LocalPtyProvider } from '../providers/local-pty-provider' import type { IPtyProvider, PtySpawnOptions, PtySpawnResult } from '../providers/types' -import { mintPtySessionId, isSafePtySessionId } from '../daemon/pty-session-id' +import { mintPtySessionIdWithOrigin, isSafePtySessionId } from '../daemon/pty-session-id' import type { ClaudeRuntimeAuthPreparation } from '../claude-accounts/runtime-auth-service' import { CLAUDE_AUTH_ENV_VARS, hasClaudeAuthEnvConflict } from '../claude-accounts/environment' import { @@ -631,6 +631,13 @@ export function registerPtyHandlers( worktreeId?: string sessionId?: string shellOverride?: string + // Why: renderer threads tabId + leafId for every daemon-host spawn so + // the main process can mint a deterministic sessionId from + // (worktreeId, tabId, leafId). That makes cold-restore survive a + // SIGKILL between spawn and the renderer's debounced persistence. + // Both fields are optional for back-compat with non-pane callers. + tabId?: string + leafId?: string } ) => { const provider = getProvider(args.connectionId) @@ -680,8 +687,40 @@ export function registerPtyHandlers( // an existing PTY whose state (OpenCode hooks, Pi overlay, agent-hook // pane caches) we must not clobber on a retry/attach failure. const isMintedSessionId = args.sessionId === undefined && isDaemonHostSpawn - const effectiveSessionId = - args.sessionId ?? (isDaemonHostSpawn ? mintPtySessionId(args.worktreeId) : undefined) + // Why: mint deterministically from (worktreeId, tabId, leafId) when + // the renderer threads those. Explicit sessionIds (reattach) and + // random-fallback (tabId/leafId missing) are recorded so we can + // spot renderer plumbing regressions in the logs. + const mintResult = isDaemonHostSpawn + ? mintPtySessionIdWithOrigin({ + worktreeId: args.worktreeId, + tabId: args.tabId, + leafId: args.leafId, + explicitSessionId: args.sessionId + }) + : args.sessionId + ? { sessionId: args.sessionId, derivedFrom: 'explicit' as const } + : null + const effectiveSessionId = mintResult?.sessionId + if (mintResult && isDaemonHostSpawn) { + const logPayload = { + worktreeId: args.worktreeId, + tabId: args.tabId, + leafId: args.leafId, + sessionId: mintResult.sessionId, + derivedFrom: mintResult.derivedFrom + } + if (mintResult.derivedFrom === 'random-fallback') { + // Why: a daemon-host spawn arriving without tabId/leafId means the + // renderer plumbing didn't thread them — cold restore will only + // survive if the renderer later persists this sessionId before a + // crash. That's exactly the race Issue #217 documented, so surface + // it as a warning. + console.warn('[pty] mintPtySessionId random-fallback', logPayload) + } else { + console.info('[pty] mintPtySessionId', logPayload) + } + } const baseEnv = claudeAuth ? { ...args.env, ...claudeAuth.envPatch } : args.env let env: Record | undefined = baseEnv const preAllocatedHandle = @@ -753,6 +792,15 @@ export function registerPtyHandlers( if (effectiveSessionId !== undefined) { spawnOptions.sessionId = effectiveSessionId } + // Why: forward tabId/leafId to the provider so any fallback mint path + // inside the adapter (tests, direct callers) can still produce a + // deterministic id. + if (args.tabId !== undefined) { + spawnOptions.tabId = args.tabId + } + if (args.leafId !== undefined) { + spawnOptions.leafId = args.leafId + } // Why: on Windows, fall back to the persisted default-shell setting // when the renderer didn't send a per-tab override. Without this, the // daemon path ignores the user's "Default Shell" preference entirely — diff --git a/src/main/providers/types.ts b/src/main/providers/types.ts index c2d07d8d6cf..4ee1f63a0f8 100644 --- a/src/main/providers/types.ts +++ b/src/main/providers/types.ts @@ -25,6 +25,15 @@ export type PtySpawnOptions = { /** Daemon session ID for reattach. When provided, the daemon reconnects * to an existing session instead of creating a new one. */ sessionId?: string + /** Tab identity, forwarded by the renderer for daemon-host spawns so the + * main process can derive a deterministic sessionId from + * (worktreeId, tabId, leafId). Closes the SIGKILL-between-spawn-and- + * persist race that loses scrollback on unclean restart. Optional for + * backwards compatibility with non-pane callers; when absent the daemon + * path falls back to a random-suffix sessionId and logs at warn level. */ + tabId?: string + /** Pane leaf identity (e.g. "pane:1"). See `tabId` for why. */ + leafId?: string /** Why: allows the renderer to request a specific shell for a single new * terminal tab (e.g. "open this tab in WSL" from the "+" submenu) without * changing the user's persistent default shell setting. Only consulted on diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index f687cb8d36f..6cbdf610fe7 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -418,6 +418,12 @@ export type PreloadApi = { // Preserved from the deleted index.d.ts PtyApi duplicate during the // single-source-of-truth collapse (see docs/preload-typecheck-hole.md §1). shellOverride?: string + // Why: renderer threads these for daemon-host spawns so the main + // process can mint a deterministic sessionId from (worktreeId, + // tabId, leafId). Closes the SIGKILL-between-spawn-and-persist race + // that lost terminal scrollback on unclean restart (Issue #217). + tabId?: string + leafId?: string }) => Promise<{ id: string snapshot?: string diff --git a/src/preload/index.ts b/src/preload/index.ts index 0b4cac85b22..ef1c01d5cdb 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -328,6 +328,8 @@ const api = { worktreeId?: string sessionId?: string shellOverride?: string + tabId?: string + leafId?: string }): Promise<{ id: string snapshot?: string diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index ce27e9a7b00..ad17eb71520 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -445,12 +445,25 @@ export function connectPanePty( // has no equivalent mechanism, so the renderer must inject it via sendInput. let pendingStartupCommand = connectionId ? (paneStartup?.command ?? null) : null + // Why: this must match the leafId the persistence layer uses for + // terminalLayoutsByTabId[tabId].ptyIdsByLeafId so a restart reaches + // the same history directory. `restoredLeafId` wins when the pane + // was materialized from a persisted snapshot (replayTerminalLayout + // preserved it); otherwise the newly-assigned paneId is the leaf. + const effectiveLeafId = deps.restoredLeafId ?? paneLeafId(pane.id) + const startFreshSpawn = (): void => { const spawnPromise = Promise.resolve( transport.connect({ url: '', cols, rows, + // Why: threading tabId/leafId lets the main process derive a + // deterministic sessionId (see src/main/daemon/pty-session-id.ts) + // so cold-restore works even if the renderer was SIGKILLed + // before its debounced ptyId persistence ran. + tabId: deps.tabId, + leafId: effectiveLeafId, callbacks: { onData: dataCallback, onReplayData: replayDataCallback, @@ -797,6 +810,12 @@ export function connectPanePty( cols, rows, sessionId: deferredReattachSessionId, + // Why: tabId/leafId are ignored when sessionId is explicit, but + // threading them keeps the audit trail intact and makes a future + // fallback (e.g. the explicit id was invalid) produce a + // deterministic id instead of a random one. + tabId: deps.tabId, + leafId: effectiveLeafId, callbacks: { onData: dataCallback, onReplayData: replayDataCallback, diff --git a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts index ba2aaa0eff3..b47b4de2257 100644 --- a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts +++ b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts @@ -153,6 +153,13 @@ export type PtyTransport = { /** Daemon session ID for reattach. When provided, the daemon reconnects * to an existing session instead of creating a new one. */ sessionId?: string + /** Tab identity, forwarded with every daemon-host spawn so the main + * process can mint a deterministic sessionId from (worktreeId, tabId, + * leafId). Closes the SIGKILL-between-spawn-and-persist race that + * lost terminal scrollback on unclean restart (Issue #217). */ + tabId?: string + /** Pane leaf identity (e.g. "pane:1"). See `tabId` for why. */ + leafId?: string callbacks: { onConnect?: () => void onDisconnect?: () => void diff --git a/src/renderer/src/components/terminal-pane/pty-transport.ts b/src/renderer/src/components/terminal-pane/pty-transport.ts index f8cb7f7f233..133b16ba01a 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.ts @@ -353,7 +353,12 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra ...(connectionId ? { connectionId } : {}), ...(options.sessionId ? { sessionId: options.sessionId } : {}), worktreeId, - ...(shellOverride ? { shellOverride } : {}) + ...(shellOverride ? { shellOverride } : {}), + // Why: thread tabId/leafId so the main process can derive a + // deterministic sessionId. Issue #217 regression prevention — + // see pty-session-id.ts. + ...(options.tabId ? { tabId: options.tabId } : {}), + ...(options.leafId ? { leafId: options.leafId } : {}) }) const spawnResult = result as PtyConnectResult & { isReattach?: boolean }