From 38d095b92d441dc0d7b7d30891075b1fcba4b9fb Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Thu, 10 Sep 2026 21:13:11 -0700 Subject: [PATCH] fix(runtime): repair a noncanonical session name instead of dropping the record A stored record that fails validation is discarded whole, so one conversationName that is not already its canonical normalization costs the session its lease, its options and its provider-handle chain. Widening the normalizer by one codepoint would turn that into a session-destroying migration. Read repair now normalizes the field before the strict validator runs: a name that still normalizes to text is repaired to canonical form, keeping the durable 'already named' marker that avoids a needless re-naming model call; one that normalizes to nothing is the only case that drops the field. Anything structural still quarantines the whole record. The store is marked for rewrite so the repaired shape persists, and the record's business updatedAt is left alone. --- .../agent-session-record-read-repair.test.ts | 185 ++++++++++++++++++ .../agent-session-record-read-repair.ts | 72 +++++++ .../agent-session-record-store-file.ts | 26 +-- 3 files changed, 265 insertions(+), 18 deletions(-) create mode 100644 src/main/runtime/agent-session-record-read-repair.test.ts create mode 100644 src/main/runtime/agent-session-record-read-repair.ts diff --git a/src/main/runtime/agent-session-record-read-repair.test.ts b/src/main/runtime/agent-session-record-read-repair.test.ts new file mode 100644 index 00000000000..cfc07dac95d --- /dev/null +++ b/src/main/runtime/agent-session-record-read-repair.test.ts @@ -0,0 +1,185 @@ +// Read repair: a recoverable optional field must never cost the session its lease, options and +// provider-handle chain. Anything structural still quarantines the whole record. + +import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import type { AgentSessionRecord } from '../../shared/agent-session-record' +import { AgentSessionRecordStore } from './agent-session-record-store' +import { agentSessionStorePath, loadAgentSessionStore } from './agent-session-record-store-file' +import type { AgentSessionReserveRequest } from './agent-session-reservation-admission' + +const NOW = 1_800_000_000_000 +const SESSION = 'session-alpha' + +let directory: string +let counter = 0 + +function operationId(): string { + counter += 1 + return `${NOW}-${String(counter) + .padStart(32, '0') + .replaceAll(/[^0-9a-f]/g, '0')}` +} + +const reserveRequest = (): AgentSessionReserveRequest => ({ + sessionId: SESSION, + location: { + executionHostId: 'local', + wslDistro: null, + workspaceId: 'workspace-1', + workspaceKind: 'git-worktree' + }, + provider: 'claude', + accountHome: { variable: 'CLAUDE_CONFIG_DIR', path: '/home/dev/.claude-work' }, + runtimeKind: 'native', + expectedFence: null, + spawnToken: 'spawn-a', + claimKeyId: 'key-1', + handoffOperationId: null, + probe: { outcome: 'indeterminate', reason: 'no answer' }, + operation: { callerKey: 'client-1', operationId: operationId(), fingerprint: 'fp-1' }, + now: NOW +}) + +/** Reserve, observe the spawn, prove the handle, carry options — a fully furnished record. */ +async function establishOwnedRecord(): Promise { + const store = await AgentSessionRecordStore.open({ directory, hostId: 'local' }) + const reserved = await store.reserveOwner(reserveRequest()) + const fence = reserved.record.lease.runtimeFence + await store.commitProcessIdentity({ + sessionId: SESSION, + fence, + process: { + hostId: 'local', + pid: 4242, + processStartTimeMs: 1_700_000_000_000, + spawnToken: reserved.record.lease.reservedSpawnToken ?? 'spawn-a' + }, + now: NOW + }) + return store.proveOwner({ + sessionId: SESSION, + fence, + link: { + linkId: 'link-1', + handle: { provider: 'claude', sessionId: 'provider-session-1', leafUuid: 'leaf-1' }, + origin: 'created', + mintedAtFence: fence, + observedAt: NOW + }, + options: { model: 'opus' }, + now: NOW + }) +} + +/** Write a name the single writer would have normalized away, as a stale build could have left it. */ +async function writeStoredName(name: unknown): Promise { + const filePath = agentSessionStorePath(directory) + const raw = JSON.parse(await readFile(filePath, 'utf-8')) + raw.records[SESSION].conversationName = name + await writeFile(filePath, JSON.stringify(raw)) +} + +async function corruptStoredLease(): Promise { + const filePath = agentSessionStorePath(directory) + const raw = JSON.parse(await readFile(filePath, 'utf-8')) + raw.records[SESSION].lease.runtimeFence = 'not-a-number' + await writeFile(filePath, JSON.stringify(raw)) +} + +const load = () => loadAgentSessionStore(agentSessionStorePath(directory), 'local') + +beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), 'orca-session-read-repair-')) +}) + +afterEach(async () => { + await rm(directory, { recursive: true, force: true }) +}) + +describe('agent session record read repair', () => { + it('repairs a noncanonical name instead of discarding the record around it', async () => { + const owned = await establishOwnedRecord() + await writeStoredName('Fix\nthe lease probe') + + const loaded = await load() + + const record = loaded.state.records.get(SESSION) + expect(record?.conversationName).toBe('Fix the lease probe') + expect(loaded.state.unreadableRecords.has(SESSION)).toBe(false) + // The fields quarantine would have taken with it. + expect(record?.lease).toMatchObject({ claimStatus: 'live', runtimeFence: 1 }) + expect(record?.options).toEqual({ model: 'opus' }) + expect(record?.providerHandleChain).toEqual(owned.providerHandleChain) + }) + + it('drops only the field when the stored name normalizes to nothing', async () => { + await establishOwnedRecord() + await writeStoredName('\u202E\u200B ') + + const loaded = await load() + + const record = loaded.state.records.get(SESSION) + expect(record).toBeDefined() + expect(record?.conversationName).toBeUndefined() + expect(Object.hasOwn(record ?? {}, 'conversationName')).toBe(false) + expect(record?.lease.claimStatus).toBe('live') + expect(record?.options).toEqual({ model: 'opus' }) + }) + + it('drops a name of the wrong type rather than the record', async () => { + await establishOwnedRecord() + await writeStoredName(42) + + const loaded = await load() + + expect(loaded.state.records.get(SESSION)?.conversationName).toBeUndefined() + expect(loaded.state.unreadableRecords.has(SESSION)).toBe(false) + }) + + it('still quarantines the whole record when the lease is structurally invalid', async () => { + await establishOwnedRecord() + await writeStoredName('Fix\nthe lease probe') + await corruptStoredLease() + // No committed copy may vouch for the session, or salvage would mask the quarantine. + await rm(`${agentSessionStorePath(directory)}.bak`, { force: true }) + + const loaded = await load() + + expect(loaded.state.records.has(SESSION)).toBe(false) + expect(loaded.state.unreadableRecords.get(SESSION)?.reason).toBe('current_shape_invalid') + // Quarantined bytes stay verbatim: repair must not edit the row it could not rescue. + expect(loaded.state.unreadableRecords.get(SESSION)?.raw).toMatchObject({ + conversationName: 'Fix\nthe lease probe' + }) + }) + + it('leaves a canonical name alone and asks for no rewrite', async () => { + const store = await AgentSessionRecordStore.open({ directory, hostId: 'local' }) + await store.reserveOwner(reserveRequest()) + await store.setConversationName(SESSION, 'Fix the lease probe') + + const loaded = await load() + + expect(loaded.state.records.get(SESSION)?.conversationName).toBe('Fix the lease probe') + expect(loaded.needsRewrite).toBe(false) + }) + + it('marks the store for rewrite without touching the record business timestamp', async () => { + const owned = await establishOwnedRecord() + await writeStoredName('Fix\nthe lease probe') + + const loaded = await load() + expect(loaded.needsRewrite).toBe(true) + expect(loaded.state.records.get(SESSION)?.updatedAt).toBe(owned.updatedAt) + + // Opening the store persists that rewrite, so the next load has nothing left to repair. + await AgentSessionRecordStore.open({ directory, hostId: 'local' }) + const persisted = JSON.parse(await readFile(agentSessionStorePath(directory), 'utf-8')) + expect(persisted.records[SESSION].conversationName).toBe('Fix the lease probe') + expect(persisted.records[SESSION].updatedAt).toBe(owned.updatedAt) + expect((await load()).needsRewrite).toBe(false) + }) +}) diff --git a/src/main/runtime/agent-session-record-read-repair.ts b/src/main/runtime/agent-session-record-read-repair.ts new file mode 100644 index 00000000000..a2f2b9bbf6d --- /dev/null +++ b/src/main/runtime/agent-session-record-read-repair.ts @@ -0,0 +1,72 @@ +/** + * Read one stored agent-session record, repairing recoverable optional metadata before validation. + * + * Quarantine is whole-record: a rejected row loses its lease, its options and its provider-handle + * chain, not only the field that failed. `conversationName` is validated against its canonical + * normalization, so widening the normalizer by one codepoint would retroactively invalidate every + * name already on disk — and take those sessions with it. Repairing the field to canonical form + * keeps the record, and keeps the durable "Orca already named this session" marker that stops a + * later acquisition from spending another model call on a name it already has. + * + * Identity and ownership stay out of reach: an invalid lease, session id or handle chain still + * quarantines the whole record, because nothing here can tell what the right value would have been. + */ + +import { normalizeAgentSessionConversationName } from '../../shared/agent-session-conversation-name' +import { + AGENT_SESSION_RECORD_SCHEMA_VERSION, + isAgentSessionRecord, + type AgentSessionRecord +} from '../../shared/agent-session-record' + +export type AgentSessionRecordReadResult = + | { record: AgentSessionRecord; repaired: boolean } + | { record: null; reason: string } + +/** Repaired shallow copy, or null when the stored field is already canonical or absent. The + * original object is left untouched so a row that fails validation anyway quarantines verbatim. */ +function repairConversationName(value: unknown): Record | null { + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + return null + } + const stored = value as Record + if (!Object.hasOwn(stored, 'conversationName')) { + return null + } + // Same predicate the validator applies, so repair and validation can never disagree. + const normalized = normalizeAgentSessionConversationName(stored.conversationName) + if (normalized === stored.conversationName) { + return null + } + const repaired = { ...stored } + if (normalized === null) { + delete repaired.conversationName + } else { + repaired.conversationName = normalized + } + return repaired +} + +export function readAgentSessionRecord( + sessionId: string, + value: unknown +): AgentSessionRecordReadResult { + const repaired = repairConversationName(value) + const candidate: unknown = repaired ?? value + const record = isAgentSessionRecord(candidate) ? candidate : null + if (record?.sessionId === sessionId) { + return { record, repaired: repaired !== null } + } + const storedSchemaVersion = + typeof value === 'object' && + value !== null && + (value as { schemaVersion?: unknown }).schemaVersion + return { + record: null, + reason: record + ? 'record_key_session_id_mismatch' + : storedSchemaVersion === AGENT_SESSION_RECORD_SCHEMA_VERSION + ? 'current_shape_invalid' + : 'unsupported_schema' + } +} diff --git a/src/main/runtime/agent-session-record-store-file.ts b/src/main/runtime/agent-session-record-store-file.ts index 7eb3cacdc7c..01b0a59ae78 100644 --- a/src/main/runtime/agent-session-record-store-file.ts +++ b/src/main/runtime/agent-session-record-store-file.ts @@ -15,17 +15,14 @@ import { isAgentSessionOperationRow, type AgentSessionOperationRow } from '../../shared/agent-session-operation-ledger' -import { - AGENT_SESSION_RECORD_SCHEMA_VERSION, - isAgentSessionRecord, - type AgentSessionRecord -} from '../../shared/agent-session-record' +import type { AgentSessionRecord } from '../../shared/agent-session-record' import { copyFileDurable, durableWriteTempPath, renameDurable, writeTempFileDurable } from '../durable-file-write' +import { readAgentSessionRecord } from './agent-session-record-read-repair' import { parseVisibleSessionIds } from './agent-session-visible-tab-index' import { serializeAgentSessionStoreState } from './agent-session-store-serialization' @@ -144,20 +141,13 @@ function parseState( let needsRewrite = false if (typeof file.records === 'object' && file.records !== null) { for (const [sessionId, value] of Object.entries(file.records)) { - const record = isAgentSessionRecord(value) ? value : null - if (record?.sessionId === sessionId) { - state.records.set(sessionId, record) + const read = readAgentSessionRecord(sessionId, value) + if (read.record) { + state.records.set(sessionId, read.record) + // A repaired record must be re-persisted, or the next load repairs it again. + needsRewrite ||= read.repaired && schemaVersion === AGENT_SESSION_STORE_SCHEMA_VERSION } else { - const valueSchemaVersion = - typeof value === 'object' && - value !== null && - (value as { schemaVersion?: unknown }).schemaVersion - const reason = record - ? 'record_key_session_id_mismatch' - : valueSchemaVersion === AGENT_SESSION_RECORD_SCHEMA_VERSION - ? 'current_shape_invalid' - : 'unsupported_schema' - state.unreadableRecords.set(sessionId, { reason, raw: value }) + state.unreadableRecords.set(sessionId, { reason: read.reason, raw: value }) needsRewrite ||= schemaVersion === AGENT_SESSION_STORE_SCHEMA_VERSION } }