-
Notifications
You must be signed in to change notification settings - Fork 29
refactor: isolate agent record persistence #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f4400a7
1c47279
bb3d171
6c496cf
198e2e4
38c985b
82d05cf
2a5ecf5
627b8d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| "@moonshot-ai/agent-core": patch | ||
| "@moonshot-ai/kimi-code": patch | ||
| --- | ||
|
|
||
| Move wire metadata handling into the record layer and keep persistence backends limited to storage operations. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,203 @@ | ||
| import { createReadStream } from 'node:fs'; | ||
| import { mkdir, open } from 'node:fs/promises'; | ||
| import { dirname } from 'node:path'; | ||
|
|
||
| import { syncDir } from '../../utils/fs'; | ||
| import { type AgentRecord, type AgentRecordPersistence } from './types'; | ||
|
|
||
| export interface FileSystemAgentRecordPersistenceOptions { | ||
| readonly onError?: ((error: unknown) => void) | undefined; | ||
| } | ||
|
|
||
| export class InMemoryAgentRecordPersistence implements AgentRecordPersistence { | ||
| readonly records: AgentRecord[] = []; | ||
|
|
||
| constructor(records: readonly AgentRecord[] = []) { | ||
| this.records.push(...records); | ||
| } | ||
|
|
||
| async *read(): AsyncIterable<AgentRecord> { | ||
| for (const record of this.records) { | ||
| yield record; | ||
| } | ||
| } | ||
|
|
||
| append(input: AgentRecord): void { | ||
| this.records.push(input); | ||
| } | ||
|
|
||
| rewrite(records: readonly AgentRecord[]): void { | ||
| this.records.splice(0, this.records.length, ...records); | ||
| } | ||
|
|
||
| async flush(): Promise<void> {} | ||
|
|
||
| async close(): Promise<void> {} | ||
| } | ||
|
|
||
| export class FileSystemAgentRecordPersistence implements AgentRecordPersistence { | ||
| private readonly pendingRecords: AgentRecord[] = []; | ||
| private shouldClear = false; | ||
| private directorySynced = false; | ||
| private flushPromise: Promise<void> | undefined; | ||
| private error: unknown; | ||
|
|
||
| constructor( | ||
| private readonly filePath: string, | ||
| private readonly options: FileSystemAgentRecordPersistenceOptions = {}, | ||
| ) {} | ||
|
|
||
| async *read(): AsyncIterable<AgentRecord> { | ||
| await this.flush(); | ||
|
|
||
| let line = ''; | ||
| let lineNumber = 0; | ||
| const stream = createReadStream(this.filePath, { encoding: 'utf8' }); | ||
| try { | ||
| for await (const chunk of stream) { | ||
| line += chunk; | ||
| let newlineIndex = line.indexOf('\n'); | ||
| while (newlineIndex !== -1) { | ||
| const rawLine = line.slice(0, newlineIndex); | ||
| line = line.slice(newlineIndex + 1); | ||
| lineNumber++; | ||
|
|
||
| const record = parseRecordLine( | ||
| rawLine.endsWith('\r') ? rawLine.slice(0, -1) : rawLine, | ||
| lineNumber, | ||
| this.filePath, | ||
| false, | ||
| ); | ||
| if (record !== undefined) yield record; | ||
|
|
||
| newlineIndex = line.indexOf('\n'); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code === 'ENOENT') return; | ||
| throw error; | ||
| } | ||
|
|
||
| if (line.length > 0) { | ||
| lineNumber++; | ||
| const record = parseRecordLine(line, lineNumber, this.filePath, true); | ||
| if (record !== undefined) yield record; | ||
| } | ||
| } | ||
|
|
||
| append(input: AgentRecord): void { | ||
| this.throwIfError(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The filesystem backend now latches write failures ( Useful? React with 👍 / 👎. |
||
| this.pendingRecords.push(input); | ||
| this.scheduleFlush(); | ||
| } | ||
|
|
||
| rewrite(records: readonly AgentRecord[]): void { | ||
| this.throwIfError(); | ||
| this.shouldClear = true; | ||
| this.pendingRecords.splice(0, this.pendingRecords.length, ...records); | ||
| this.scheduleFlush(); | ||
| } | ||
|
|
||
| async flush(): Promise<void> { | ||
| this.throwIfError(); | ||
| while ( | ||
| this.flushPromise !== undefined || | ||
| this.shouldClear || | ||
| this.pendingRecords.length > 0 | ||
| ) { | ||
| await this.ensureFlush(); | ||
| this.throwIfError(); | ||
| } | ||
| } | ||
|
|
||
| async close(): Promise<void> { | ||
| await this.flush(); | ||
| } | ||
|
|
||
| private scheduleFlush(): void { | ||
| void this.ensureFlush().catch((error) => { | ||
| this.options.onError?.(error); | ||
| }); | ||
| } | ||
|
|
||
| private ensureFlush(): Promise<void> { | ||
| if (this.flushPromise !== undefined) return this.flushPromise; | ||
|
|
||
| const promise = this.drainPendingRecords() | ||
| .catch((error: unknown) => { | ||
| this.error = error; | ||
| // oxlint-disable-next-line typescript-eslint/only-throw-error | ||
| throw error; | ||
| }) | ||
| .finally(() => { | ||
| if (this.flushPromise === promise) { | ||
| this.flushPromise = undefined; | ||
| } | ||
| if ( | ||
| this.error === undefined && | ||
| (this.shouldClear || this.pendingRecords.length > 0) | ||
| ) { | ||
| this.scheduleFlush(); | ||
| } | ||
| }); | ||
| this.flushPromise = promise; | ||
| return promise; | ||
| } | ||
|
|
||
| private throwIfError(): void { | ||
| // oxlint-disable-next-line typescript-eslint/only-throw-error | ||
| if (this.error !== undefined) throw this.error; | ||
| } | ||
|
|
||
| private async drainPendingRecords(): Promise<void> { | ||
| while (this.shouldClear || this.pendingRecords.length > 0) { | ||
| await this.drainBatch(); | ||
| } | ||
| } | ||
|
|
||
| private async drainBatch(): Promise<void> { | ||
| const shouldClear = this.shouldClear; | ||
| const batch = this.pendingRecords.splice(0); | ||
| this.shouldClear = false; | ||
|
|
||
| const content = batch.map((e) => JSON.stringify(e) + '\n').join(''); | ||
| const directory = dirname(this.filePath); | ||
| await mkdir(directory, { recursive: true }); | ||
|
|
||
| const fh = await open(this.filePath, shouldClear ? 'w' : 'a'); | ||
| try { | ||
| if (content.length > 0) { | ||
| await fh.writeFile(content, 'utf8'); | ||
| } | ||
| await fh.sync(); | ||
| } finally { | ||
| await fh.close(); | ||
| } | ||
|
|
||
| if (!this.directorySynced) { | ||
| await syncDir(directory); | ||
| this.directorySynced = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function parseRecordLine( | ||
| line: string, | ||
| lineNumber: number, | ||
| filePath: string, | ||
| allowTruncated: boolean, | ||
| ): AgentRecord | undefined { | ||
| if (line.length === 0) return undefined; | ||
| try { | ||
| return JSON.parse(line) as AgentRecord; | ||
| } catch (parseError) { | ||
| // Tolerate a truncated trailing line — last write may have crashed | ||
| // mid-flush; everything before is still well-formed. | ||
| if (allowTruncated) return undefined; | ||
| throw new Error( | ||
| `wire.jsonl: corrupted line ${lineNumber} in ${filePath}: ${String(parseError)}`, | ||
| { cause: parseError }, | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,11 @@ import type { UserToolRegistration } from '../tool'; | |
| import type { UsageRecordScope } from '../usage'; | ||
|
|
||
| export interface AgentRecordEvents { | ||
| metadata: { | ||
| protocol_version: string; | ||
| created_at: number; | ||
| }; | ||
|
|
||
| 'turn.prompt': { | ||
| input: readonly ContentPart[]; | ||
| origin: PromptOrigin; | ||
|
|
@@ -85,7 +90,8 @@ export const AGENT_WIRE_PROTOCOL_VERSION = '1.0'; | |
|
|
||
| export interface AgentRecordPersistence { | ||
| read(): AsyncIterable<AgentRecord>; | ||
| append(input: AgentRecord): Promise<void>; | ||
| append(input: AgentRecord): void; | ||
| rewrite(records: readonly AgentRecord[]): void; | ||
|
Comment on lines
+93
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎. |
||
| flush(): Promise<void>; | ||
| close(): Promise<void>; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AgentRecords.logRecordnow callspersistence.appendsynchronously and ignores its return value, so a custom persistence passed viaAgentConfig.persistencethat still implementsappendas async (the pre-change contract) can reject without being observed. In that case record-write failures are no longer routed through the existing error path and may surface as unhandled promise rejections while turn APIs continue as if persistence succeeded, which is a regression from the previousappend(...).catch(...)behavior.Useful? React with 👍 / 👎.