From c802e2690a44628c5f85252198eb99a83ee20ea4 Mon Sep 17 00:00:00 2001 From: AlexRV12 <71396855+AlexRV12@users.noreply.github.com> Date: Tue, 1 Sep 2026 19:00:17 +0200 Subject: [PATCH] fix: land a run whose updates endpoint fails, and anchor the run form's drawers --- .../copilot/chat/AIChatManager.svelte.ts | 10 ++- .../copilot/chat/AIChatManager.test.ts | 22 +++++- .../components/copilot/chat/shared.test.ts | 41 ++++++++-- .../src/lib/components/copilot/chat/shared.ts | 78 +++++++++++-------- .../components/sessions/PreviewTabHost.svelte | 12 ++- 5 files changed, 120 insertions(+), 43 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index db29e0eae9..b487bac64b 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -1021,10 +1021,12 @@ export class AIChatManager { } const update = await reader.poll() if (gen !== this.#jobPollGeneration) return - this.applyToolStatus(job.toolCallId, { - logs: update.logs || undefined, - resultStream: update.resultStream || undefined - }) + if (update) { + this.applyToolStatus(job.toolCallId, { + logs: update.logs || undefined, + resultStream: update.resultStream || undefined + }) + } const fetched = await JobService.getJob({ workspace: job.workspace, diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts index 4e26bbb055..919b710a45 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts @@ -36,6 +36,7 @@ const mocks = vi.hoisted(() => ({ runChatLoop: vi.fn(), listResource: vi.fn(), getJob: vi.fn(), + getJobUpdates: vi.fn(), whoami: vi.fn(), workspace: 'test_workspace' as string | undefined, // The workspace being browsed, which a session chat's own workspace need not be. @@ -60,7 +61,8 @@ vi.mock('$lib/gen', () => ({ whoami: mocks.whoami }, JobService: { - getJob: mocks.getJob + getJob: mocks.getJob, + getJobUpdates: mocks.getJobUpdates } })) @@ -172,6 +174,10 @@ beforeEach(() => { mocks.getOpenaiClient.mockReturnValue({}) mocks.getAnthropicClient.mockReturnValue({}) mocks.listResource.mockResolvedValue([]) + // Re-seeded here rather than in the factory: clearAllMocks keeps implementations, so a + // test that makes the updates endpoint fail would otherwise leave it failing for the rest + // of the file. Neutral by default — completion is getJob's answer. + mocks.getJobUpdates.mockResolvedValue({ completed: false, running: true }) mocks.workspace = 'test_workspace' mocks.runChatLoop.mockResolvedValue({ addedMessages: [], @@ -3986,6 +3992,20 @@ describe('AIChatManager background job completion', () => { expect((manager.displayMessages[0] as any).isLoading).toBe(false) }) + // Streaming rides on a second endpoint; landing the job must not. A poll that always + // fails would otherwise spend the failure budget and drain a job that finished, leaving + // the card on "unreachable". + it('completes a job whose updates endpoint keeps failing', async () => { + const manager = new AIChatManager() + manager.registerJob(datatableJob) + mocks.getJobUpdates.mockRejectedValue(new Error('updates unavailable')) + mocks.getJob.mockResolvedValue(completed({ result: [{ n: 1 }] })) + + await completeDetachedJob(manager) + + expect(manager.backgroundJobs[0]?.status).toBe('success') + }) + it('reconstructs the datatable result contract from the persisted resultFormat', async () => { const manager = new AIChatManager() manager.registerJob(datatableJob) diff --git a/frontend/src/lib/components/copilot/chat/shared.test.ts b/frontend/src/lib/components/copilot/chat/shared.test.ts index 095855645e..768b6d1b65 100644 --- a/frontend/src/lib/components/copilot/chat/shared.test.ts +++ b/frontend/src/lib/components/copilot/chat/shared.test.ts @@ -1434,19 +1434,50 @@ describe('pollJobCompletion detach', () => { const { JobService } = await import('$lib/gen') const getJob = vi.mocked(JobService.getJob) getJob.mockReset() - const completed = { type: 'CompletedJob', success: true, result: 42 } + const completed = { type: 'CompletedJob', success: true, result: 42, logs: 'ran' } getJob.mockResolvedValue(completed as any) - // The updates endpoint is what says the job landed; the whole job is then - // fetched once, with its logs. + // Still landing on a tick the updates endpoint calls unfinished: the job can + // complete between the two calls, and `getJob` is what says so. const getJobUpdates = vi.mocked(JobService.getJobUpdates) getJobUpdates.mockReset() - getJobUpdates.mockResolvedValue({ completed: true } as any) + getJobUpdates.mockResolvedValue({ completed: false, running: true } as any) const cbs = makeCallbacks() const promise = pollJobCompletion('job1', 'w', 'tool1', cbs as any, { detachAfterMs: 15000 }) await vi.advanceTimersByTimeAsync(1000) - expect(await promise).toBe(completed) + const landed = await promise + expect(landed).toBe(completed) + // Fetched again with its logs rather than settled on the logless tick fetch, + // which would reach the model as "No logs available". + expect((landed as any).logs).toBe('ran') + } finally { + vi.useRealTimers() + } + }) + + // Streaming rides on a second endpoint; landing the job must not. A failing updates + // endpoint costs live logs, never the run. + it('returns the completed job with its logs when the updates endpoint fails', async () => { + vi.useFakeTimers() + try { + const { pollJobCompletion } = await import('./shared') + const { JobService } = await import('$lib/gen') + const getJob = vi.mocked(JobService.getJob) + getJob.mockReset() + const completed = { type: 'CompletedJob', success: true, result: 42, logs: 'ran' } + getJob.mockResolvedValue(completed as any) + const getJobUpdates = vi.mocked(JobService.getJobUpdates) + getJobUpdates.mockReset() + getJobUpdates.mockRejectedValue(new Error('updates unavailable')) + const cbs = makeCallbacks() + + const promise = pollJobCompletion('job1', 'w', 'tool1', cbs as any, { detachAfterMs: 15000 }) + await vi.advanceTimersByTimeAsync(1000) + + const landed = await promise + expect(landed).toBe(completed) + expect((landed as any).logs).toBe('ran') } finally { vi.useRealTimers() } diff --git a/frontend/src/lib/components/copilot/chat/shared.ts b/frontend/src/lib/components/copilot/chat/shared.ts index 77a10e7171..361b3fc4b1 100644 --- a/frontend/src/lib/components/copilot/chat/shared.ts +++ b/frontend/src/lib/components/copilot/chat/shared.ts @@ -1633,7 +1633,11 @@ export type BackgroundJobFormatter = (job: CompletedJob) => { /** Reads a running job's output incrementally through `getJobUpdates`, the only endpoint * carrying `new_result_stream`: `getJob` returns logs but never the partial result. Both the * inline wait and the background poller drive one, so a detached run keeps streaming, and - * each keeps its own offsets so one starting over refetches from zero. */ + * each keeps its own offsets so one starting over refetches from zero. + * + * Best-effort by construction: a poll that fails answers `undefined` rather than throwing, so + * a run always lands on `getJob` alone. Nothing is mutated before the response arrives, so the + * next poll resumes from the same offsets. */ export function createJobUpdateReader(jobId: string, workspace: string) { let logs = '' let resultStream = '' @@ -1641,14 +1645,19 @@ export function createJobUpdateReader(jobId: string, workspace: string) { let streamOffset = 0 let started = false return { - async poll(): Promise<{ completed: boolean; logs: string; resultStream: string }> { - const update = await JobService.getJobUpdates({ - workspace, - id: jobId, - running: started, - logOffset, - streamOffset - }) + async poll(): Promise<{ completed: boolean; logs: string; resultStream: string } | undefined> { + let update: Awaited> + try { + update = await JobService.getJobUpdates({ + workspace, + id: jobId, + running: started, + logOffset, + streamOffset + }) + } catch { + return undefined + } started ||= update.running ?? false // Both kept as a tail: the offsets come from the server, so dropping the head // costs nothing here, and neither is the record of the run — the logs are on the @@ -1694,36 +1703,43 @@ export async function pollJobCompletion( // The tray's snapshot is trimmed of logs (it is persisted), so the card is the // only place a running job's output can land. Cards that hide their logs while // loading are unaffected; the run card follows them line by line. - toolCallbacks.setToolStatus(toolId, { - logs: formatLogs(update.logs), - resultStream: update.resultStream || undefined - }) - - if (update.completed) { - // Fetched whole rather than assembled from the ticks: the reader stops at - // whatever the last one saw, and the tail written between then and the job - // landing is only on the job itself. - const completed = await JobService.getJob({ - workspace: workspace, - id: jobId, - noLogs: false, - noCode: true + if (update) { + toolCallbacks.setToolStatus(toolId, { + logs: formatLogs(update.logs), + resultStream: update.resultStream || undefined }) - if (completed.type === 'CompletedJob') { - job = completed - break - } } - // Keeps the tray's status + Job snapshot fresh during the inline wait. Its logs - // are skipped because the reader above already has them; the badge needs the real - // Job to tell running from suspended or scheduled, which the updates do not say. + // Ask for the logs when the run may be over — the tail written between the last + // poll and the end is only on the job itself — or when there is no reader output + // to have collected them. + const wantLogs = !update || update.completed const fetchedJob = await JobService.getJob({ workspace: workspace, id: jobId, - noLogs: true, + noLogs: !wantLogs, noCode: true }) + if (fetchedJob.type === 'CompletedJob') { + // The updates can still call a landed job unfinished, so a completion seen on + // a logless fetch is fetched again rather than settled without them: the model + // reads these logs, and their absence is indistinguishable from a silent run. + job = wantLogs + ? fetchedJob + : ((await JobService.getJob({ + workspace: workspace, + id: jobId, + noLogs: false, + noCode: true + })) as CompletedJob) + break + } + // With no reader, this is the only place the card's logs can come from. + if (!update) { + toolCallbacks.setToolStatus(toolId, { logs: formatLogs(fetchedJob.logs) }) + } + // The badge needs the real Job to tell running from suspended or scheduled, which + // the updates do not say. toolCallbacks.onJobStatus?.(jobId, { status: deriveChatJobStatus(fetchedJob), job: trimJob(fetchedJob) diff --git a/frontend/src/lib/components/sessions/PreviewTabHost.svelte b/frontend/src/lib/components/sessions/PreviewTabHost.svelte index 9a18319f8f..3dee7dd1b3 100644 --- a/frontend/src/lib/components/sessions/PreviewTabHost.svelte +++ b/frontend/src/lib/components/sessions/PreviewTabHost.svelte @@ -329,8 +329,16 @@ {/if} {:else if slot.kind === 'runform' && mounted} -
- {#if runtime} +
+ + {#if runtime && overlayHostEl} {/if}