🗂️ fix: Keep a Running Attachment Chat in the Sidebar - #16104
Conversation
Since #15694, a turn with model-bound attachments defers its user-message write until the model admits the payload. That write was also what created a new conversation's row, so for the whole first model call, which with a long generated script or extended thinking can run for minutes, the conversation did not exist. Any conversation-list refetch in that window (starting another chat, deleting one, reloading) dropped the running chat from the sidebar until its run finished, and GET /api/convos/:id answered 404 for it. When attachments are the only reason to defer, the new conversation's row is now written as the run starts, carrying the same fields the message write would set but no message. The message itself stays deferred until admission, and its write waits for a seed still in flight so the two upserts never race. A content policy still holds back every write, the row included, as #14425 intended. The conversation half of saveMessageToDatabase moves into saveConversationToDatabase so the seed and the message write share one path.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 0939353adb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (this.shouldSeedDeferredConversation()) { | ||
| const conversationSeed = this.seedConversation(userMessage.conversationId, saveOptions); |
There was a problem hiding this comment.
/api
This branch adds persistence orchestration, state handling, and a database service call directly to the legacy CJS client, with the new helpers below adding still more behavior in the same layer. The repository explicitly requires branches, helpers, and service calls added under /api—including edits to existing CJS files—to live in packages/api; move this workflow into a TypeScript module and leave only the wiring call here.
AGENTS.md reference: AGENTS.md:L51-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in da93efe. The conversation half of a turn's writes now lives in packages/api/src/conversations/save.ts: saveTurnConversation, seedTurnConversation, runAfterSeed, and the retention context builder. The module takes getConvo/saveConvo from the caller. BaseClient keeps only the wiring: the synchronous retention lookup that Stop depends on, the message save, and the calls into the module. save.spec.ts covers the module against the real data-schemas methods on an in-memory MongoDB.
| initialAgentId: hasNonEphemeralAgent ? options.agent?.id : null, | ||
| createdAtOnInsert: shouldSetCreatedAtOnInsert ? validCreatedAtOnInsert : undefined, | ||
| ...(savedMessage?._id != null ? { appendMessageIds: [savedMessage._id] } : {}), | ||
| ...(savedMessageId != null ? { appendMessageIds: [savedMessageId] } : {}), |
There was a problem hiding this comment.
For every new attachment-deferred chat, savedMessageId is absent here, so appendMessageIds is omitted; packages/data-schemas/src/methods/conversation.ts then takes its appendMessageIds == null path and awaits getMessages(...) before performing the upsert. The seed is known to contain no message, making this a guaranteed extra serial Mongo read on the startup/file path and delaying the very sidebar visibility this change is intended to restore; use a seed-specific data-schema operation or explicitly signal an empty append set.
AGENTS.md reference: AGENTS.md:L38-L43
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in da93efe. The seed passes appendMessageIds: [], which takes saveConvo's no-read path, so the write never loads the message list. save.spec.ts › "creates the row a deferred first message has not written yet" spies on Message.find and asserts it is never called. The test fails if the seed goes back to omitting the append set. The seed also resolves the conversation once and leaves it on the request, and "lets the deferred message save reuse the seeded row" asserts that the later message save makes no second getConvo read.
The conversation half of a turn's writes (the row a message save upserts, the retention context it stamps, and the new seed ahead of a deferred first message) moves into `conversations/save.ts`, which takes `getConvo`/`saveConvo` from the caller. BaseClient keeps only the wiring: the retention lookup that must stay synchronous for Stop, the message save, and the calls into the module. The seed passes an empty `appendMessageIds`, which tells `saveConvo` the row holds no messages yet, so it no longer reads the message list before the upsert. It also resolves the conversation once and leaves it on the request, so the deferred message save reuses it instead of looking the conversation up again. save.spec.ts drives the real data-schemas methods against an in-memory MongoDB: the seed creates an empty row without reading messages, the message save appends to it without a second lookup, an existing chat and a subagent thread are left alone, a temporary chat keeps its retention, and a failed lookup settles instead of rejecting.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
A new agent chat started with an attachment disappears from the sidebar while its run is in flight, and comes back only when the run finishes.
GET /api/convos/:idanswers 404 for it the whole time. v0.8.7 did not do this.The cause is #15694. Since that change, a turn with model-bound attachments defers its user-message write until the model admits the payload, which happens after the starting agent's first model call. That same write was what created a new conversation's row. The first model call can run for minutes when it thinks at length or generates a long script for code execution, and the row does not exist during that time. Any conversation-list refetch in that window drops the running chat, and nothing restores it until another refetch. Refetches come from starting another chat, deleting one, reloading the page, or another run finishing.
After this change, when attachments are the only reason to defer, the new conversation's row is written as the run starts. It carries the same fields the message write would set, but no message. The message itself stays deferred until admission, exactly as before. When a content policy (
filtersormessageFilter.pii) is active, every write is still held back, the row included, as #14425 intended.Fixes #16045
How it works
The conversation half of a turn's writes lives in
packages/api/src/conversations/save.ts, which takesgetConvo/saveConvofrom the caller. The seed and the message save sharewriteConversation, so the seeded row carries exactly the fields the message save would set. The message save waits for a seed that is still in flight, so the two upserts on one row never race. Once the seed has landed, a Stop still starts the parent write synchronously.Type of change
Testing
The reporter's logs show a Bedrock agent with Code Interpreter and an
.xlsx.GET /api/convos/:idreturned 404 for six minutes while the job endpoints returned 200 for the same id. With this change the row exists before the first model call, so the list and the direct lookup both return it.I also checked the admission timing against the real SDK graph, with the stock and the Bedrock Converse model classes. The deferred write starts after the starting agent's first model call, before any tool runs. So even when admission works as designed, the missing-row window is the whole first model call.
Automated tests:
packages/api/src/conversations/save.spec.tsruns against the real data-schemas methods on an in-memory MongoDB. It covers these cases:runAfterSeedordering.api/app/clients/specs/BaseClient.test.jscovers these cases:api/server/controllers/agents/client.test.js: an attachment-only deferral seeds, and a content policy does not.BaseClient.test.jsandclient.test.jssuites, therequest.resumeMetadataspec, and the agent controllerclient*/request*specs (10 suites).Risk / compatibility