mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
fix: only refuse an abort when a REPLACEMENT actually holds the conversation
The 409 I added for a lost generation fence was too broad. abortJob reports `success: false, jobData: null` in two different situations: a replacement turn claimed the conversationId, and the job simply vanished between the lookup and the abort. Only the first can be damaged by the side effects below — pruning a replacement's checkpoint, or writing a partial for a generation still running. The second is the benign race of pressing Stop as a turn completes, and turning that into an error is a regression I introduced. The refusal is now gated on a DIFFERENT generation actually being present. Both directions are covered, and the vanished-job test fails against the broad form. Found by auditing this session's own changes for the shape that has already bitten twice today: a fix that removes one hazard and silently introduces its mirror.
This commit is contained in:
parent
85a0cfe2b8
commit
0b05e206e4
2 changed files with 63 additions and 9 deletions
|
|
@ -515,6 +515,51 @@ describe('Agent Abort Endpoint', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('Replacement vs vanished job', () => {
|
||||
/**
|
||||
* Both cases surface as `success: false, jobData: null`, but they need opposite
|
||||
* handling: a REPLACEMENT must not have its checkpoint pruned, while a job that
|
||||
* merely vanished (Stop pressed as the generation completes) is a benign race the
|
||||
* user should not see an error for.
|
||||
*/
|
||||
it('refuses when a DIFFERENT generation now holds the conversation', async () => {
|
||||
mockGenerationJobManager.getJob
|
||||
.mockResolvedValueOnce({ metadata: { userId: 'test-user-123' }, createdAt: 1000 })
|
||||
.mockResolvedValue({ metadata: { userId: 'test-user-123' }, createdAt: 2000 });
|
||||
mockGenerationJobManager.abortJob.mockResolvedValue({
|
||||
success: false,
|
||||
jobData: null,
|
||||
content: [],
|
||||
});
|
||||
|
||||
const response = await request(app)
|
||||
.post('/api/agents/chat/abort')
|
||||
.send({ conversationId: 'conv-1' });
|
||||
|
||||
expect(response.status).toBe(409);
|
||||
// Nothing downstream ran: no partial was written for the live replacement.
|
||||
expect(mockSaveMessage).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('still succeeds when the job simply vanished mid-abort', async () => {
|
||||
mockGenerationJobManager.getJob
|
||||
.mockResolvedValueOnce({ metadata: { userId: 'test-user-123' }, createdAt: 1000 })
|
||||
.mockResolvedValue(null);
|
||||
mockGenerationJobManager.abortJob.mockResolvedValue({
|
||||
success: false,
|
||||
jobData: null,
|
||||
content: [],
|
||||
});
|
||||
|
||||
const response = await request(app)
|
||||
.post('/api/agents/chat/abort')
|
||||
.send({ conversationId: 'conv-1' });
|
||||
|
||||
// Nothing to damage, so pressing Stop as a turn finishes stays a quiet success.
|
||||
expect(response.status).toBe(200);
|
||||
});
|
||||
});
|
||||
|
||||
describe('Scheduled runs', () => {
|
||||
const scheduledJob = {
|
||||
metadata: {
|
||||
|
|
|
|||
|
|
@ -379,16 +379,25 @@ router.post('/chat/abort', configMiddleware, async (req, res) => {
|
|||
abortResultResponseMessageId: abortResult.jobData?.responseMessageId,
|
||||
});
|
||||
|
||||
// LOST THE FENCE: a replacement turn claimed this conversationId between the lookup
|
||||
// above and the abort, so nothing of ours was stopped. Everything below acts on the
|
||||
// conversation as a whole — pruning the checkpoint would strip the REPLACEMENT's
|
||||
// resume state, and persisting `abortResult` content would write a partial for a
|
||||
// generation that is still running. Report it as not-aborted instead.
|
||||
// LOST THE FENCE to a REPLACEMENT: another turn claimed this conversationId between
|
||||
// the lookup above and the abort. Everything below acts on the conversation as a
|
||||
// whole — pruning the checkpoint would strip the replacement's resume state, and
|
||||
// persisting `abortResult` content would write a partial for a generation that is
|
||||
// still running. Refuse instead.
|
||||
//
|
||||
// Deliberately gated on a replacement actually being there. abortJob also reports
|
||||
// `success: false, jobData: null` when the job simply VANISHED between the lookup
|
||||
// and the abort — the benign race of pressing Stop as a generation completes. There
|
||||
// is nothing to damage in that case, so it keeps its previous behaviour rather than
|
||||
// turning a routine stop into an error.
|
||||
if (!abortResult.success && abortResult.jobData == null) {
|
||||
logger.debug(
|
||||
`[AgentStream] Abort refused: generation was replaced before it landed: ${jobStreamId}`,
|
||||
);
|
||||
return res.status(409).json({ error: 'This generation was superseded', aborted: null });
|
||||
const liveJob = await GenerationJobManager.getJob(jobStreamId).catch(() => null);
|
||||
if (liveJob != null && liveJob.createdAt !== job.createdAt) {
|
||||
logger.debug(
|
||||
`[AgentStream] Abort refused: generation was replaced before it landed: ${jobStreamId}`,
|
||||
);
|
||||
return res.status(409).json({ error: 'This generation was superseded', aborted: null });
|
||||
}
|
||||
}
|
||||
|
||||
// HITL: prune the durable checkpoint of a run aborted while paused, so a new turn
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue