-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
🧷 fix: Finalize Stopped Compactions and Verify Their Anchor #16551
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
c7bd2d1
4808452
82f630a
0cf711e
4970dab
d0cc1b5
a8b5b0f
6ef4a08
6709a54
ef67844
f2dceb4
3a70382
d4e8dfb
c725b37
1cfb300
e0f8e94
73acf4a
9eca66c
4a96922
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 |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ const { | |
| TERMINAL_PUBLICATION_RECONNECT_ERROR, | ||
| hasPersistableAbortContent, | ||
| announceStoppedReply, | ||
| resolveAbortedTurnPersistence, | ||
| buildAbortedResponseMetadata, | ||
| isPendingActionStale, | ||
| toClientPendingAction, | ||
|
|
@@ -56,6 +57,7 @@ const { | |
| } = require('~/server/controllers/agents/protocol'); | ||
| const { | ||
| getFiles, | ||
| getMessages, | ||
| saveMessage, | ||
| saveConvo, | ||
| getPersistedPrivateTextId, | ||
|
|
@@ -789,15 +791,21 @@ router.post('/chat/abort', chatConfigMiddleware, async (req, res, next) => { | |
| * its parent and the preliminary-parent fence correctly rejects it. */ | ||
| const shouldPersistAbortedTurn = | ||
| hasPersistableAbortContent(content) || jobData?.createdEventEmitted === true; | ||
| /** A compaction's `userMessage` is the already-persisted leaf | ||
| * projected for identity only; upserting it would erase a user | ||
| * leaf's text or turn an assistant leaf into an empty user row. */ | ||
| const shouldPersistAnchor = jobData?.compact !== true; | ||
| /** The stopped turn's persistence plan (which rows to write, and | ||
| * whether the normal FINAL must be withheld for a reconciliation | ||
| * frame instead) comes from @librechat/api, decided from the | ||
| * compaction anchor this route reads. */ | ||
| const abortPersistencePlan = await resolveAbortedTurnPersistence( | ||
| jobData, | ||
| shouldPersistAbortedTurn, | ||
| { userId: req?.user?.id, getMessages }, | ||
| ); | ||
| persistenceErrors.push(...abortPersistencePlan.persistenceErrors); | ||
|
|
||
| if ( | ||
| jobData?.userMessage?.messageId && | ||
| jobData?.responseMessageId && | ||
| shouldPersistAbortedTurn | ||
| abortPersistencePlan.writeResponseRow | ||
|
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.
When a compaction with a persisted anchor is stopped, this branch writes the response with AGENTS.md reference: AGENTS.md:L49-L52 Useful? React with 👍 / 👎.
Collaborator
Author
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. Fixed in c95e57e. The abort plan now carries responseUnfinished (false for a compaction, true for an ordinary turn) and the route writes it. The e2e scenario stopped-compaction-settles-under-its-anchor reproduced this on fbaf453 (stored row unfinished: true on all three projects) and passes on 170672d; stopped-reply-keeps-turn-and-partial-answer covers the ordinary path. |
||
| ) { | ||
| const messageContext = { | ||
| userId: req?.user?.id, | ||
|
|
@@ -827,7 +835,7 @@ router.post('/chat/abort', chatConfigMiddleware, async (req, res, next) => { | |
| endpoint: jobData.endpoint, | ||
| iconURL: jobData.iconURL, | ||
| model: jobData.model, | ||
| unfinished: true, | ||
| unfinished: abortPersistencePlan.responseUnfinished, | ||
|
berry-13 marked this conversation as resolved.
|
||
| error: false, | ||
| isCreatedByUser: false, | ||
| ...(Array.isArray(jobData.userSubmittedPaths) && | ||
|
|
@@ -859,7 +867,7 @@ router.post('/chat/abort', chatConfigMiddleware, async (req, res, next) => { | |
| * operation gets a chance to succeed. A compaction skips the | ||
| * prerequisite: its anchor is the persisted leaf itself. */ | ||
| let persistedRequestId; | ||
| if (shouldPersistAnchor) { | ||
| if (abortPersistencePlan.writeUserRow) { | ||
| try { | ||
| const persistedRequest = await saveAbortedUserMessage( | ||
| { saveMessage, getPersistedPrivateTextId, getPrivateMessageTexts }, | ||
|
|
||
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.
When a disconnected compaction has already saved a partial row and generation then fails,
settleExistingRowsBeforeErrorTurnfinalizes that row and this early return skips bothsaveConvoandannounceErrorTurnbelow. ConsequentlylastResponseAtis not stamped and another device has no notification that the persisted turn ended, even though the ordinary error-row path explicitly performs that announcement. Announce the finalized live row before returning so failure and restored-session behavior remain consistent.AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
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.
Fixed in c95e57e. settleExistingRowsBeforeErrorTurn takes an injected announceSettledTurn and calls it only when it actually finalized the live row; request.js passes announceErrorTurn with stampConvoLastResponse. compaction.spec.ts covers both the announce and the no-write case.