refactor: isolate agent record persistence#14
Conversation
kermanx
force-pushed
the
codex/refactor-agent-record-persistence
branch
from
ec2bbca to
38c985b
Compare
1 similar comment
|
[P1] Persistence failures can now synchronously abort later agent state updates. After FileSystemAgentRecordPersistence enters its error state, append() throws synchronously; AgentRecords.logRecord() is called before many live in-memory mutations, such as TurnFlow.prompt() launching the turn and ContextMemory.appendMessage() updating history. This means one background wire.jsonl write failure can make later prompts/context/tool updates throw before the agent updates its live state, instead of only reporting records.write_failed and surfacing the failure through flush()/shutdown paths. Consider catching persistence append failures in AgentRecords and preserving the old non-blocking live path while still reporting the error and making flush() fail. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a5ecf5425
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (stamped.type === 'metadata') { | ||
| this.metadataInitialized = true; | ||
| } | ||
| this.persistence?.append(stamped); |
There was a problem hiding this comment.
AgentRecords.logRecord now calls persistence.append synchronously and ignores its return value, so a custom persistence passed via AgentConfig.persistence that still implements append as 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 previous append(...).catch(...) behavior.
Useful? React with 👍 / 👎.
| append(input: AgentRecord): void; | ||
| rewrite(records: readonly AgentRecord[]): void; |
There was a problem hiding this comment.
AgentRecordPersistence is exported from the public agent API, and this change makes rewrite mandatory while changing append from Promise<void> to void. Any external persistence implementation compiled against the previous contract will now fail type-checking (missing rewrite) or silently change behavior around async append completion, so this is a breaking API change that should be compatibility-gated rather than shipped as a patch-level contract swap.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| append(input: AgentRecord): void { | ||
| this.throwIfError(); |
There was a problem hiding this comment.
The filesystem backend now latches write failures (this.error) and append() throws synchronously on every later call via throwIfError(). Because AgentRecords.logRecord calls append without a try/catch, any subsequent prompt/config/tool action that logs a record can fail with a raw filesystem exception instead of only emitting records.write_failed, effectively making the agent unusable after a transient I/O failure.
Useful? React with 👍 / 👎.
2 participants
Related Issue
N/A
Description
Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.