🫙 fix: Preserve Loaded Message Content When Resume Snapshot Is Empty#14399
Conversation
The sync handler guarded on `data.resumeState?.aggregatedContent` and then assigned it unconditionally. An empty array is truthy, so a resume snapshot carrying no content overwrote the content the messages query had already loaded, leaving the message rendering as a bare cursor with no way to recover. Treat an empty snapshot as non-authoritative: keep the loaded content and let live deltas take over. A snapshot that carries content still wins, unchanged. This is defense in depth rather than a root cause. A resume snapshot goes empty when the conversation's job is replaced mid-flight (#14348) — because `streamId === conversationId`, a second submission calls `createJob` on the same key while the first run is still streaming. With this change that failure degrades to stale-but-visible instead of destroying content already on screen.
There was a problem hiding this comment.
Pull request overview
Fixes a client-side resumable-stream sync edge case where an empty resumeState.aggregatedContent snapshot could overwrite already-loaded message content (from the messages query), causing the UI to render a “bare cursor” response with no recoverable text.
Changes:
- Treat empty resume snapshots as non-authoritative by preserving the existing message
contentwhenaggregatedContent.length === 0. - Add Jest coverage to ensure empty snapshots preserve loaded content and non-empty snapshots still replace content as before.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| client/src/hooks/SSE/useResumableSSE.ts | Preserve existing loaded message content when a resume sync snapshot has empty aggregatedContent; add diagnostic log field. |
| client/src/hooks/SSE/tests/useResumableSSE.spec.ts | Add tests covering empty vs non-empty resume snapshot reconciliation behavior. |
Comments suppressed due to low confidence (1)
client/src/hooks/SSE/tests/useResumableSSE.spec.ts:1314
- This test flushes microtasks with an inline
act(Promise.resolve())block even though the file already defines aflushMicrotasks()helper. Using the helper reduces repetition and keeps async timing consistent across tests.
await act(async () => {
await Promise.resolve();
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return { submission, chatHelpers }; | ||
| }; | ||
|
|
||
| const emitSync = async (aggregatedContent: unknown[]) => { |
| await act(async () => { | ||
| await Promise.resolve(); | ||
| }); |
|
@codex review |
![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: bbe97858cb
ℹ️ 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".
| const responseMessage = { | ||
| ...messages[responseIdx], | ||
| content: data.resumeState.aggregatedContent, | ||
| content: hasResumedContent ? data.resumeState.aggregatedContent : oldContent, |
There was a problem hiding this comment.
When a user reloads during an in-flight regenerate before the new run has aggregated any content, the server can report an empty aggregatedContent with a padded responseMessageId, while the loaded DB history still contains the old assistant response under the same parent. This fallback can select that old response, and this line now seeds syncStepMessage with its old content; subsequent text deltas append to that stale answer instead of starting the regenerated response from blank.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in 7915c15.
You're right that the parent-based fallback can land on the answer being regenerated: it matches any !isCreatedByUser row whose parentMessageId === userMsgId, and on a regenerate the new run's responseMessageId isn't in the loaded history yet, so that fallback is exactly what fires.
Preservation is now scoped to rows matched by the server's declared responseMessageId, which is the only case that proves the row belongs to this generation:
let matchedByResponseId = false;
if (serverResponseId) {
responseIdx = messages.findIndex((m) => m.messageId === serverResponseId);
matchedByResponseId = responseIdx >= 0;
}
// ...
const preserveLoadedContent = !hasResumedContent && matchedByResponseId;A fallback-matched row keeps the previous clear-on-empty behavior, so a regenerated run still starts blank. This also keeps the original fix effective for the case it targets, since a resumed placeholder and a persisted partial both carry the server's response id.
Added does not preserve content on a row matched only by the parent fallback to cover it; verified it fails against the previous version of the guard and passes now.
Codex P2: on a regenerate reload before the new run aggregated anything, the server reports an empty snapshot under a response id not yet in the loaded history. The parent-based fallback then lands on the answer being REPLACED, and preserving its content seeded `syncStepMessage` with the stale response, so the regenerated run's deltas appended to it instead of starting blank. Preserve loaded content only when the row was matched by the server's declared `responseMessageId` — the sole case proving the row belongs to this generation. A fallback-matched row keeps the previous clear-on-empty behavior.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
danny-avila
changed the title
A matched row with no `content` array would have been assigned `undefined` instead of the snapshot's array. `MultiMessage` branches on `message.content` truthiness to pick its renderer, and `[]` is truthy while `undefined` is not, so that would have silently switched a streaming row from the content-parts renderer to the text one. Preserve only when the loaded row has a non-empty content array — the case the guard exists for. Narrows the divergence from prior behavior further: it now applies solely when there is real content to protect.
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
Summary
The resumable-stream sync handler guarded on
data.resumeState?.aggregatedContentand then assigned it unconditionally. An empty array is truthy, so a resume snapshot carrying no content overwrote content already loaded for that same response — the message then rendered as a bare cursor even though the conversation's own history had the text.An empty snapshot is now treated as non-authoritative for a response we can identify, and the loaded content is kept.
Scope
Behavior differs from
devonly when both conditions hold:resumeState.responseMessageIdmatch, andaggregatedContentis emptyIn that case the loaded content is preserved instead of cleared. Every other path is unchanged — non-empty snapshots, the
elsebranch that creates a new response row, and any row located by the parent-based fallback all behave exactly as before. The change can only ever skip a write that previously happened; it never writes anything new.Concretely, this covers a response whose partial was persisted under the same id and whose snapshot subsequently comes back empty. It is a robustness guard on the resume path, not a fix for any one root cause: an empty snapshot on a live run is itself worth investigating separately (see the note below), and this only ensures that when it happens, already-visible content is not destroyed.
Why preservation is restricted to an id match
Regenerate makes the parent-based fallback unsafe. The client derives the regenerated response id by padding the previous one (
useChatFunctions.ts):and the server adopts that value verbatim (
getPreliminaryResponseMessageIdinrequest.js). So mid-regenerate the job carriesoldRespId_while loaded history still holdsoldRespId. The exact-id lookup cannot match, the fallback fires, and it selects the answer being replaced. Preserving that row's content would seedsyncStepMessagewith the stale response and make the regenerated run's deltas append to it.Requiring an id match is what makes preservation safe: it is the only condition proving the row belongs to the generation the snapshot describes. Deliberately not done here: teaching the lookup to also try the unpadded id. That would widen behavior on this hot path without evidence demanding it, and regenerate is precisely where that would go wrong.
Thanks @chatgpt-codex-connector for catching the regenerate interaction — it was a real regression in the first revision of this PR.
Related
A resume snapshot can go empty when a conversation's job is replaced mid-flight: because
streamId === conversationId, a second submission callscreateJobon the same key while the first run is still streaming. On the in-memory job store the first run's content then becomes unreachable (appendChunkis gated behindif (this._isRedis), andsetContentPartsreplaces the stored array reference), sogetResumeStatereportsaggregatedContent: []andrunSteps: []whileactivestaystrue. On Redis the same replacement is survivable, sinceJOB_CREATE_LUAclears only the steer keys and the chunk log is reconstructed.That is the ambiguity tracked in #14348, whose clean fix (per-generation stream ids) remains open. This PR does not address it — a replacement carries its own
responseMessageId, so the preserve path above does not apply to it.Test plan
Three tests in
useResumableSSE.spec.ts:Each guard verified non-vacuous by reverting the corresponding change and confirming the test fails, then passes with it restored.
npx jest src/hooks/SSE→ 7 suites, 149 tests passing.