From 0b05e206e4df9f4472b2b1fb45d08a5bd37a2fc6 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 27 Jul 2026 11:59:19 -0400 Subject: [PATCH] fix: only refuse an abort when a REPLACEMENT actually holds the conversation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../routes/agents/__tests__/abort.spec.js | 45 +++++++++++++++++++ api/server/routes/agents/index.js | 27 +++++++---- 2 files changed, 63 insertions(+), 9 deletions(-) diff --git a/api/server/routes/agents/__tests__/abort.spec.js b/api/server/routes/agents/__tests__/abort.spec.js index 1c4c70d06d..eb49e56c07 100644 --- a/api/server/routes/agents/__tests__/abort.spec.js +++ b/api/server/routes/agents/__tests__/abort.spec.js @@ -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: { diff --git a/api/server/routes/agents/index.js b/api/server/routes/agents/index.js index 4ae6279bac..452e558708 100644 --- a/api/server/routes/agents/index.js +++ b/api/server/routes/agents/index.js @@ -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