diff --git a/frontend/src/components/chat/clarification-form.test.tsx b/frontend/src/components/chat/clarification-form.test.tsx index 7f9f8e2b6b..bd416ccea6 100644 --- a/frontend/src/components/chat/clarification-form.test.tsx +++ b/frontend/src/components/chat/clarification-form.test.tsx @@ -226,3 +226,398 @@ describe("ClarificationForm Session file capability", () => { expect(container.querySelector('input[type="file"]')).not.toBeNull() }) }) + +describe("ClarificationForm delivery failures", () => { + beforeEach(() => { + appContextMock.dispatch.mockReset() + appContextMock.filesDisabled = false + appContextMock.providerAvailable = true + appContextMock.sendMessage.mockReset() + toastErrorMock.mockReset() + }) + + afterEach(() => { + cleanup() + }) + + const deliveryError = ( + message: string, + disposition: string, + userFacing = false, + ) => Object.assign(new Error(message), { disposition, userFacing }) + + const submitAnswer = async (onSend: ReturnType) => { + render( + , + ) + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + fireEvent.click( + screen.getByRole("button", { name: "chatPage.clarification.submit" }), + ) + } + + it("surfaces the backend rejection reason instead of the generic toast", async () => { + const onSend = vi.fn().mockRejectedValue(deliveryError( + "A previous guidance message is still being applied. Please wait for it to finish.", + "rejected", + true, + )) + + await submitAnswer(onSend) + + await waitFor(() => { + expect(toastErrorMock).toHaveBeenCalledWith( + "A previous guidance message is still being applied. Please wait for it to finish.", + { description: "chatPage.clarification.sendNotSent" }, + ) + }) + expect(await screen.findByRole("alert")).toHaveTextContent( + "A previous guidance message is still being applied.", + ) + }) + + it("keeps the form submittable after a failure that never reached the agent", async () => { + const onSend = vi.fn().mockRejectedValue( + deliveryError("Durable storage is temporarily unavailable", "not_sent", true), + ) + + await submitAnswer(onSend) + + await waitFor(() => expect(toastErrorMock).toHaveBeenCalledWith( + "Durable storage is temporarily unavailable", + { description: "chatPage.clarification.sendNotSent" }, + )) + const submit = screen.getByRole("button", { + name: "chatPage.clarification.submit", + }) + expect(submit).toBeEnabled() + expect(screen.getByRole("textbox")).toHaveValue("Beijing") + }) + + it("tells the sender a delivery it could not confirm is safe to repeat", async () => { + const onSend = vi.fn().mockRejectedValue(deliveryError( + "The task is busy applying an earlier answer.", + "outcome_unknown", + true, + )) + + await submitAnswer(onSend) + + await waitFor(() => { + expect(toastErrorMock).toHaveBeenCalledWith( + "The task is busy applying an earlier answer.", + { description: "chatPage.clarification.sendDeliveryUnconfirmed" }, + ) + }) + }) + + it("asks for a reload only when an attachment may need reconciling", async () => { + const onSend = vi.fn().mockRejectedValue(Object.assign( + new Error("The upload could not be completed or rolled back."), + { disposition: "outcome_unknown", userFacing: true, requiresReconciliation: true }, + )) + + await submitAnswer(onSend) + + await waitFor(() => { + expect(toastErrorMock).toHaveBeenCalledWith( + "The upload could not be completed or rolled back.", + { description: "chatPage.clarification.sendOutcomeUnknown" }, + ) + }) + }) + + it("keeps connection plumbing diagnostics away from the visitor", async () => { + const onSend = vi.fn().mockRejectedValue(deliveryError( + "Message not sent: the connection changed before delivery.", + "not_sent", + )) + + await submitAnswer(onSend) + + await waitFor(() => { + expect(toastErrorMock).toHaveBeenCalledWith( + "chatPage.clarification.sendError", + { description: "chatPage.clarification.sendNotSent" }, + ) + }) + expect(await screen.findByRole("alert")).not.toHaveTextContent( + "the connection changed before delivery", + ) + }) + + it("falls back to the generic string when the failure carries no reason", async () => { + const onSend = vi.fn().mockRejectedValue(new Error(" ")) + + await submitAnswer(onSend) + + await waitFor(() => { + expect(toastErrorMock).toHaveBeenCalledWith( + "chatPage.clarification.sendError", + undefined, + ) + }) + }) + + it("clears the failure once the visitor edits an answer", async () => { + const onSend = vi.fn().mockRejectedValue( + deliveryError("Durable storage is temporarily unavailable", "not_sent", true), + ) + + await submitAnswer(onSend) + + await screen.findByRole("alert") + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Shanghai" } }) + await waitFor(() => expect(screen.queryByRole("alert")).toBeNull()) + }) +}) + +describe("ClarificationForm resubmission safety", () => { + beforeEach(() => { + appContextMock.dispatch.mockReset() + appContextMock.filesDisabled = false + appContextMock.providerAvailable = true + appContextMock.sendMessage.mockReset() + toastErrorMock.mockReset() + }) + + afterEach(() => { + cleanup() + }) + + const deliveryError = ( + message: string, + disposition: string, + extra: Record = {}, + ) => Object.assign(new Error(message), { disposition, userFacing: true, ...extra }) + + const renderForm = () => render( + , + ) + + + const submit = () => fireEvent.click( + screen.getByRole("button", { name: "chatPage.clarification.submit" }), + ) + + const sentIds = () => appContextMock.sendMessage.mock.calls.map( + ([, config]) => (config as { clientMessageId?: string })?.clientMessageId, + ) + + it("stops a second submission when an attachment may already have landed", async () => { + // Uploaded bytes have no server-side dedup, so this is the one case a + // human has to reconcile before sending the draft again. + appContextMock.sendMessage.mockRejectedValue(deliveryError( + "The upload could not be completed or rolled back.", + "outcome_unknown", + { requiresReconciliation: true }, + )) + renderForm() + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + + submit() + + await waitFor(() => expect(screen.getByRole("button", { + name: "chatPage.clarification.submit", + })).toBeDisabled()) + // Editing an answer must not talk the visitor back into resubmitting. + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Shanghai" } }) + expect(screen.getByRole("button", { + name: "chatPage.clarification.submit", + })).toBeDisabled() + expect(screen.getByRole("alert")).toHaveTextContent( + "chatPage.clarification.sendOutcomeUnknown", + ) + expect(appContextMock.sendMessage).toHaveBeenCalledTimes(1) + }) + + it("lets an unknown delivery outcome be retried under the same id", async () => { + // A reconnect during ack-wait is an ordinary event. The turn keeps its + // client message id, so the server adjudicates a duplicate instead of the + // form locking the visitor out until they reload the page. + appContextMock.sendMessage.mockRejectedValue(deliveryError( + "Message delivery was not acknowledged. Your draft was kept.", + "outcome_unknown", + )) + renderForm() + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + + submit() + await waitFor(() => expect(appContextMock.sendMessage).toHaveBeenCalledTimes(1)) + expect(screen.getByRole("button", { + name: "chatPage.clarification.submit", + })).toBeEnabled() + // The copy must match the state: submitting again is safe here, so it + // must not tell the visitor to reload first. + expect(screen.getByRole("alert")).toHaveTextContent( + "chatPage.clarification.sendDeliveryUnconfirmed", + ) + expect(screen.getByRole("alert")).not.toHaveTextContent( + "chatPage.clarification.sendOutcomeUnknown", + ) + + submit() + await waitFor(() => expect(appContextMock.sendMessage).toHaveBeenCalledTimes(2)) + const [first, second] = sentIds() + expect(second).toBe(first) + }) + + it("clears a previous round's block when the form is asked again", async () => { + // The live turn render path keeps one instance across clarification + // rounds, so a stale block would silently disable round two. + appContextMock.sendMessage.mockRejectedValue(deliveryError( + "The upload could not be completed or rolled back.", + "outcome_unknown", + { requiresReconciliation: true }, + )) + const interactions = [{ type: "text_input" as const, field: "city", label: "City" }] + const { rerender } = render( + , + ) + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + submit() + await waitFor(() => expect(screen.getByRole("button", { + name: "chatPage.clarification.submit", + })).toBeDisabled()) + + rerender() + rerender() + + expect(screen.getByRole("button", { + name: "chatPage.clarification.submit", + })).toBeEnabled() + expect(screen.queryByRole("alert")).toBeNull() + + appContextMock.sendMessage.mockResolvedValue(undefined) + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Shanghai" } }) + submit() + await waitFor(() => expect(appContextMock.sendMessage).toHaveBeenCalledTimes(2)) + const [first, second] = sentIds() + expect(second).not.toBe(first) + }) + + it("retries an unresolved submission under its original client message id", async () => { + appContextMock.sendMessage.mockRejectedValue( + deliveryError("Durable storage is temporarily unavailable", "not_sent"), + ) + renderForm() + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + + submit() + await waitFor(() => expect(appContextMock.sendMessage).toHaveBeenCalledTimes(1)) + submit() + await waitFor(() => expect(appContextMock.sendMessage).toHaveBeenCalledTimes(2)) + + const [first, second] = sentIds() + expect(first).toBeTruthy() + expect(second).toBe(first) + }) + + it("mints a fresh client message id when the server asks for one", async () => { + appContextMock.sendMessage.mockRejectedValue(deliveryError( + "Message id was already used for different content or files.", + "rejected", + { retryWithNewId: true }, + )) + renderForm() + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + + submit() + await waitFor(() => expect(appContextMock.sendMessage).toHaveBeenCalledTimes(1)) + submit() + await waitFor(() => expect(appContextMock.sendMessage).toHaveBeenCalledTimes(2)) + + const [first, second] = sentIds() + expect(first).toBeTruthy() + expect(second).not.toBe(first) + }) +}) + +describe("ClarificationForm undecided deliveries", () => { + beforeEach(() => { + appContextMock.dispatch.mockReset() + appContextMock.filesDisabled = false + appContextMock.providerAvailable = true + appContextMock.sendMessage.mockReset() + toastErrorMock.mockReset() + }) + + afterEach(() => { + cleanup() + }) + + const submit = () => fireEvent.click( + screen.getByRole("button", { name: "chatPage.clarification.submit" }), + ) + + const sentIds = () => appContextMock.sendMessage.mock.calls.map( + ([, config]) => (config as { clientMessageId?: string })?.clientMessageId, + ) + + it("never mints a new id after an undecided attempt reached the server", async () => { + // The server only asks for a new id when a claim already exists under the + // old one - which means the undecided answer did land. Minting another + // would answer the same question twice. + appContextMock.sendMessage + .mockRejectedValueOnce(Object.assign( + new Error("The message is still being applied. Please retry shortly."), + { disposition: "outcome_unknown", userFacing: true }, + )) + .mockRejectedValueOnce(Object.assign( + new Error("Message id was already used for different content or files."), + { disposition: "rejected", userFacing: true, retryWithNewId: true }, + )) + render( + , + ) + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + + submit() + await waitFor(() => expect(appContextMock.sendMessage).toHaveBeenCalledTimes(1)) + // The form invites this: an unconfirmed delivery leaves Submit enabled. + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Shanghai" } }) + submit() + await waitFor(() => expect(appContextMock.sendMessage).toHaveBeenCalledTimes(2)) + + const [first, second] = sentIds() + expect(second).toBe(first) + // Nothing further may be sent: the answer is in an unresolved state only a + // reload can settle. + expect(screen.getByRole("button", { + name: "chatPage.clarification.submit", + })).toBeDisabled() + expect(screen.getByRole("alert")).toHaveTextContent( + "chatPage.clarification.sendOutcomeUnknown", + ) + }) + + it("still mints a new id when the server refuses one that never landed", async () => { + appContextMock.sendMessage + .mockRejectedValueOnce(Object.assign( + new Error("Message id was already used for different content or files."), + { disposition: "rejected", userFacing: true, retryWithNewId: true }, + )) + .mockResolvedValueOnce(undefined) + render( + , + ) + fireEvent.change(screen.getByRole("textbox"), { target: { value: "Beijing" } }) + + submit() + await waitFor(() => expect(appContextMock.sendMessage).toHaveBeenCalledTimes(1)) + submit() + await waitFor(() => expect(appContextMock.sendMessage).toHaveBeenCalledTimes(2)) + + const [first, second] = sentIds() + expect(second).not.toBe(first) + }) +}) diff --git a/frontend/src/components/chat/clarification-form.tsx b/frontend/src/components/chat/clarification-form.tsx index e598410755..fa0ad9836d 100644 --- a/frontend/src/components/chat/clarification-form.tsx +++ b/frontend/src/components/chat/clarification-form.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useMemo, useState } from "react" +import React, { useEffect, useMemo, useRef, useState } from "react" import { Interaction } from "@/contexts/app-context-chat" import { Input } from "@/components/ui/input" import { Button } from "@/components/ui/button" @@ -12,6 +12,7 @@ import { useI18n } from "@/contexts/i18n-context" import { toast } from "@/components/ui/sonner" import { Collapsible, CollapsibleContent, CollapsibleTrigger } from "@/components/ui/collapsible" import { ChevronDown, ChevronRight, MessageSquare, Upload, File as FileIcon, X, Globe } from "lucide-react" +import { generateClientMessageId } from "@/lib/utils" interface ClarificationFormProps { message?: string @@ -44,6 +45,43 @@ const isFileActionSelection = ( ? isFileActionOption(option) : isFileActionValue(value) +type SendDisposition = "not_sent" | "rejected" | "outcome_unknown" + +/** + * Delivery failures carry whether the turn definitely never reached the agent. + * Plain errors (local validation, unexpected throws) carry nothing, and are + * left unqualified rather than guessed at: telling a visitor to resubmit a + * turn that may have landed is worse than saying nothing. + */ +const readSendDisposition = (error: unknown): SendDisposition | null => { + if (typeof error !== "object" || error === null || !("disposition" in error)) { + return null + } + const disposition = (error as { disposition: unknown }).disposition + return disposition === "not_sent" + || disposition === "rejected" + || disposition === "outcome_unknown" + ? disposition + : null +} + +/** + * Only the reasons the sender can act on — the backend's rejection text, an + * upload response detail — are shown as-is. Connection plumbing messages stay + * behind the localized string: they are English diagnostics, and a widget + * visitor is not the audience for them. + */ +const readSendReason = (error: unknown): string => { + if ( + typeof error !== "object" + || error === null + || (error as { userFacing?: unknown }).userFacing !== true + ) { + return "" + } + return error instanceof Error ? error.message.trim() : "" +} + export function ClarificationForm({ interactions, messageId, @@ -68,11 +106,31 @@ export function ClarificationForm({ const [isSubmitting, setIsSubmitting] = useState(false) const [isSubmitted, setIsSubmitted] = useState(!active) const [isOpen, setIsOpen] = useState(active) + const [sendFailure, setSendFailure] = useState<{ message: string; hint: string | null } | null>(null) + // An unresolved submission keeps its client message id, so a retry lands on + // the server's existing claim instead of opening a second turn. `unknown` + // records that the previous attempt's fate was undecided - a fresh id is + // then never safe to mint, because the first turn may still be landing. + const deliveryAttemptRef = useRef< + { clientMessageId: string; unknown: boolean } | null + >(null) + // Set only when a resubmit could duplicate something the server cannot + // deduplicate - an attachment that may have landed under an id this client + // never learned. An unknown *delivery* outcome does not block: the retry + // reuses the same client message id, so the server adjudicates it. + const [resubmitBlocked, setResubmitBlocked] = useState(false) useEffect(() => { if (active) { + // A new clarification round reuses this component instance on the live + // turn render path, so every per-submission guard has to be cleared - + // otherwise round 1's block, or its client message id, leaks into + // round 2's answer. setIsSubmitted(false) setIsOpen(true) + setResubmitBlocked(false) + setSendFailure(null) + deliveryAttemptRef.current = null } }, [active]) @@ -154,6 +212,8 @@ export function ClarificationForm({ const handleInputChange = (field: string, value: any) => { setFormState((prev) => ({ ...prev, [field]: value })) + if (resubmitBlocked) return + setSendFailure(null) } const handleSubmit = async () => { @@ -246,6 +306,7 @@ export function ClarificationForm({ try { setIsSubmitting(true) + setSendFailure(null) // If textMessage is empty but we have files, send a generic message? const outboundFiles = filesDisabled ? [] : files const finalMessage = textMessage || (outboundFiles.length > 0 ? t("chatPage.clarification.uploadedFiles") : t("chatPage.clarification.confirmed")) @@ -253,9 +314,23 @@ export function ClarificationForm({ if (onSend) { await onSend(finalMessage, outboundFiles, metadata); } else if (sendMessage) { - await sendMessage(finalMessage, { force: true, metadata }, outboundFiles) + const previousAttempt = deliveryAttemptRef.current + const clientMessageId = previousAttempt?.clientMessageId + ?? generateClientMessageId() + // Reusing an id carries its history: an attempt whose fate was + // undecided stays undecided until something settles it. + deliveryAttemptRef.current = { + clientMessageId, + unknown: previousAttempt?.unknown ?? false, + } + await sendMessage( + finalMessage, + { force: true, metadata, clientMessageId }, + outboundFiles, + ) } + deliveryAttemptRef.current = null setIsSubmitted(true) setIsOpen(false) if (!onSend && dispatch) { @@ -263,7 +338,50 @@ export function ClarificationForm({ } } catch (error) { console.error("Failed to send clarification response", error) - toast.error(t("chatPage.clarification.sendError")) + // The rejection reason ("a previous guidance message is still being + // applied", the upload failure detail) is the only actionable part of + // the failure; the fixed string is a last resort. + const detail = readSendReason(error) + const disposition = readSendDisposition(error) + const needsReconciliation = typeof error === "object" + && error !== null + && (error as { requiresReconciliation?: unknown }).requiresReconciliation === true + const previousWasUnknown = deliveryAttemptRef.current?.unknown === true + const serverAsksForNewId = Boolean( + error + && typeof error === "object" + && (error as { retryWithNewId?: unknown }).retryWithNewId === true, + ) + // A fresh id opens a new turn. That is only safe when the previous + // attempt definitively did not land: after an undecided one, the server + // asking for a new id means our answer *did* reach it under the old one, + // and minting another would answer the same question twice. + if (serverAsksForNewId && !previousWasUnknown) { + deliveryAttemptRef.current = null + } else if (deliveryAttemptRef.current) { + deliveryAttemptRef.current = { + ...deliveryAttemptRef.current, + unknown: previousWasUnknown || disposition === "outcome_unknown", + } + } + const mustReconcile = needsReconciliation + || (serverAsksForNewId && previousWasUnknown) + const failure = { + message: detail || t("chatPage.clarification.sendError"), + // Only an attachment that may have landed needs a reload. An unknown + // delivery outcome keeps its client message id, so submitting again + // is safe and the copy must not say otherwise. + hint: mustReconcile + ? t("chatPage.clarification.sendOutcomeUnknown") + : disposition === "outcome_unknown" + ? t("chatPage.clarification.sendDeliveryUnconfirmed") + : disposition === "not_sent" || disposition === "rejected" + ? t("chatPage.clarification.sendNotSent") + : null, + } + setResubmitBlocked(mustReconcile) + setSendFailure(failure) + toast.error(failure.message, failure.hint ? { description: failure.hint } : undefined) } finally { setIsSubmitting(false) } @@ -499,8 +617,17 @@ export function ClarificationForm({ ))} + {sendFailure && ( +
+
{sendFailure.message}
+ {sendFailure.hint && ( +
{sendFailure.hint}
+ )} +
+ )} +
-
diff --git a/frontend/src/hooks/use-websocket.test.ts b/frontend/src/hooks/use-websocket.test.ts index 5c660f9b3d..61fd1b9112 100644 --- a/frontend/src/hooks/use-websocket.test.ts +++ b/frontend/src/hooks/use-websocket.test.ts @@ -7,6 +7,7 @@ import { useWebSocket, } from "./use-websocket" import { refreshStoredAccessToken } from "@/lib/api-wrapper" +import { UploadRequestError } from "@/lib/upload-retry" import { AUTH_CACHE_KEY, readAuthCache, readAuthSessionSnapshot, type AuthSessionSnapshot } from "@/lib/auth-cache" const authState = vi.hoisted(() => ({ @@ -164,6 +165,305 @@ describe("useWebSocket message delivery", () => { }) }) + const uploadResponse = (body: unknown, status: number) => new Response( + JSON.stringify(body), + { status, headers: { "Content-Type": "application/json" } }, + ) + + it("retries the default batch upload when storage refuses it, then stamps the ids", async () => { + // No injected uploader: this drives the FormData/apiRequest/parser branch + // that the transport-injected tests never reach. + const fetchMock = vi.spyOn(globalThis, "fetch") + .mockImplementationOnce(async () => uploadResponse( + { detail: "Durable storage is temporarily unavailable" }, + 503, + )) + .mockImplementationOnce(async () => uploadResponse( + { success: true, files: [{ file_id: "file-7", filename: "evidence.txt" }] }, + 200, + )) + const { result } = renderHook(() => useWebSocket({ url: "ws://localhost", taskId: 1 })) + await waitFor(() => expect(MockWebSocket.instances).toHaveLength(1)) + const socket = MockWebSocket.instances[0] + act(() => socket.open()) + const file = new File(["evidence"], "evidence.txt") as File & { file_id?: string } + + const delivery = result.current.sendChatMessage("answer", [file], false, "batch-retry") + await waitFor(() => expect(socket.send).toHaveBeenCalledTimes(1)) + act(() => socket.receive({ + type: "message_accepted", + client_message_id: "batch-retry", + turn_id: "turn-7", + })) + + await expect(delivery).resolves.toMatchObject({ turn_id: "turn-7" }) + expect(fetchMock).toHaveBeenCalledTimes(2) + expect(JSON.parse(socket.send.mock.calls[0][0] as string).files).toEqual([ + { file_id: "file-7", name: "evidence.txt", size: 0, type: "" }, + ]) + // Stamped back onto the caller's File, so resubmitting the same draft + // sends the reference instead of the bytes. + expect(file.file_id).toBe("file-7") + }) + + it("reports an ambiguous batch upload as an unknown outcome", async () => { + // A gateway timeout means the upstream got the request; the file may be + // stored under an id this client never learned, so the sender must not be + // told the draft is safe to send again. + const fetchMock = vi.spyOn(globalThis, "fetch").mockImplementation( + async () => uploadResponse({ detail: "Gateway timeout" }, 504), + ) + const { result } = renderHook(() => useWebSocket({ url: "ws://localhost", taskId: 1 })) + await waitFor(() => expect(MockWebSocket.instances).toHaveLength(1)) + act(() => MockWebSocket.instances[0].open()) + + await expect(result.current.sendChatMessage( + "answer", + [new File(["evidence"], "evidence.txt")], + false, + "batch-ambiguous", + )).rejects.toMatchObject({ + disposition: "outcome_unknown", + userFacing: true, + }) + expect(fetchMock).toHaveBeenCalledTimes(1) + expect(MockWebSocket.instances[0].send).not.toHaveBeenCalled() + }) + + it("reports a dropped batch upload as an unknown outcome", async () => { + vi.spyOn(globalThis, "fetch").mockRejectedValue(new TypeError("Failed to fetch")) + const { result } = renderHook(() => useWebSocket({ url: "ws://localhost", taskId: 1 })) + await waitFor(() => expect(MockWebSocket.instances).toHaveLength(1)) + act(() => MockWebSocket.instances[0].open()) + + await expect(result.current.sendChatMessage( + "answer", + [new File(["evidence"], "evidence.txt")], + false, + "batch-dropped", + )).rejects.toMatchObject({ disposition: "outcome_unknown" }) + }) + + it("re-decides ownership loss during an upload as an unknown outcome", async () => { + // The cancellation error is built with a fixed "not_sent" before anyone + // knows what the upload did. A reconnect while bytes are still moving is + // exactly when that is wrong: the file may land moments later. + const upload = deferred>() + let reconnect!: () => void + const hook = renderHook(() => { + const webSocket = useWebSocket({ + url: "ws://localhost", + taskId: 1, + uploadFiles: vi.fn(() => upload.promise), + onConnectionClose: () => { + reconnect() + return "handled" as const + }, + }) + reconnect = webSocket.connect + return webSocket + }) + await waitFor(() => expect(MockWebSocket.instances).toHaveLength(1)) + act(() => MockWebSocket.instances[0].open()) + const preparing = hook.result.current.sendChatMessage( + "answer", + [new File(["evidence"], "evidence.txt")], + false, + "cancelled-mid-upload", + ) + + act(() => MockWebSocket.instances[0].triggerClose(4001)) + + await expect(preparing).rejects.toMatchObject({ + disposition: "outcome_unknown", + requiresReconciliation: true, + }) + await act(async () => { + upload.resolve([{ file_id: "late-upload" }]) + await Promise.resolve() + }) + }) + + it("keeps the server's refusal when a reconnect interrupts the backoff", async () => { + // A 503 is a refusal the endpoint already rolled back. Losing that to a + // reconnect during the wait would lock the form on the exact failure this + // retry exists for. + vi.spyOn(globalThis, "fetch").mockImplementation( + async () => uploadResponse({ detail: "Durable storage is temporarily unavailable" }, 503), + ) + const hook = renderHook(() => useWebSocket({ + url: "ws://localhost", + taskId: 1, + // Hold the backoff open, so the reconnect below lands between attempts + // rather than racing the production jitter. + uploadRetry: { sleep: () => new Promise(() => {}) }, + })) + await waitFor(() => expect(MockWebSocket.instances).toHaveLength(1)) + act(() => MockWebSocket.instances[0].open()) + const preparing = hook.result.current.sendChatMessage( + "answer", + [new File(["evidence"], "evidence.txt")], + false, + "cancelled-in-backoff", + ) + await waitFor(() => expect(globalThis.fetch).toHaveBeenCalledTimes(1)) + + act(() => MockWebSocket.instances[0].triggerClose(4001)) + + await expect(preparing).rejects.toMatchObject({ + disposition: "not_sent", + requiresReconciliation: false, + }) + expect(globalThis.fetch).toHaveBeenCalledTimes(1) + }) + + it("does not let a refusal vouch for a later attempt that died on the wire", async () => { + // Attempt one is refused and rolled back; attempt two dies mid-request and + // may have committed. The second attempt must not inherit the first's + // clean bill of health. + const fetchMock = vi.spyOn(globalThis, "fetch") + .mockImplementationOnce(async () => uploadResponse( + { detail: "Durable storage is temporarily unavailable" }, + 503, + )) + .mockImplementationOnce(async () => { throw new TypeError("Failed to fetch") }) + const { result } = renderHook(() => useWebSocket({ + url: "ws://localhost", + taskId: 1, + uploadRetry: { sleep: async () => {} }, + })) + await waitFor(() => expect(MockWebSocket.instances).toHaveLength(1)) + act(() => MockWebSocket.instances[0].open()) + + await expect(result.current.sendChatMessage( + "answer", + [new File(["evidence"], "evidence.txt")], + false, + "dropped-on-retry", + )).rejects.toMatchObject({ + disposition: "outcome_unknown", + requiresReconciliation: true, + }) + expect(fetchMock).toHaveBeenCalledTimes(2) + }) + + it("rejects a batch response that does not account for every file", async () => { + // All-or-nothing endpoint: a short list means the response cannot be + // read, not that the turn should go out without its attachments. + vi.spyOn(globalThis, "fetch").mockImplementation( + async () => uploadResponse( + { success: true, files: [{ file_id: "file-1" }] }, + 200, + ), + ) + const { result } = renderHook(() => useWebSocket({ url: "ws://localhost", taskId: 1 })) + await waitFor(() => expect(MockWebSocket.instances).toHaveLength(1)) + act(() => MockWebSocket.instances[0].open()) + + await expect(result.current.sendChatMessage( + "answer", + [new File(["a"], "a.txt"), new File(["b"], "b.txt")], + false, + "batch-short", + )).rejects.toMatchObject({ disposition: "outcome_unknown" }) + expect(MockWebSocket.instances[0].send).not.toHaveBeenCalled() + }) + + it("keeps ownership loss outside an upload reported as never sent", async () => { + const hook = renderHook(() => useWebSocket({ url: "ws://localhost", taskId: 1 })) + await waitFor(() => expect(MockWebSocket.instances).toHaveLength(1)) + act(() => MockWebSocket.instances[0].open()) + const pending = hook.result.current.sendChatMessage( + "answer", + undefined, + false, + "cancelled-no-upload", + ) + await waitFor(() => expect(MockWebSocket.instances[0].send).toHaveBeenCalled()) + + act(() => MockWebSocket.instances[0].triggerClose(4001)) + + await expect(pending).rejects.toMatchObject({ + requiresReconciliation: false, + }) + }) + + it("keeps a refused upload reported as never sent", async () => { + // The server declined and kept nothing, so the draft really is safe to + // resend - the one upload failure that must not ask for reconciliation. + vi.spyOn(globalThis, "fetch").mockImplementation( + async () => uploadResponse({ detail: "File is too large" }, 413), + ) + const { result } = renderHook(() => useWebSocket({ url: "ws://localhost", taskId: 1 })) + await waitFor(() => expect(MockWebSocket.instances).toHaveLength(1)) + act(() => MockWebSocket.instances[0].open()) + + await expect(result.current.sendChatMessage( + "answer", + [new File(["evidence"], "evidence.txt")], + false, + "refused-upload", + )).rejects.toMatchObject({ + disposition: "not_sent", + requiresReconciliation: false, + }) + }) + + it("does not replay the default batch upload for a permanent rejection", async () => { + const fetchMock = vi.spyOn(globalThis, "fetch").mockImplementation( + async () => uploadResponse({ detail: "File is too large" }, 413), + ) + const { result } = renderHook(() => useWebSocket({ url: "ws://localhost", taskId: 1 })) + await waitFor(() => expect(MockWebSocket.instances).toHaveLength(1)) + act(() => MockWebSocket.instances[0].open()) + const file = new File(["evidence"], "evidence.txt") as File & { file_id?: string } + + await expect(result.current.sendChatMessage( + "answer", + [file], + false, + "batch-permanent", + )).rejects.toMatchObject({ + message: "File is too large", + disposition: "not_sent", + userFacing: true, + }) + expect(fetchMock).toHaveBeenCalledTimes(1) + expect(file.file_id).toBeUndefined() + expect(MockWebSocket.instances[0].send).not.toHaveBeenCalled() + }) + + it("keeps an upload failure's own reason user facing through delivery", async () => { + // The clarification form only shows a failure reason that is marked user + // facing; an upload detail loses its marker if this wrapping regresses, + // and the visitor drops back to the generic "Failed to send response". + const uploadFiles = vi.fn(() => Promise.reject( + new UploadRequestError("Durable storage is temporarily unavailable", { + status: 503, + retriable: true, + outcome: "refused", + }), + )) + const { result } = renderHook(() => useWebSocket({ + url: "ws://localhost", + taskId: 1, + uploadFiles, + })) + await waitFor(() => expect(MockWebSocket.instances).toHaveLength(1)) + act(() => MockWebSocket.instances[0].open()) + + await expect(result.current.sendChatMessage( + "answer", + [new File(["evidence"], "evidence.txt")], + false, + "upload-detail", + )).rejects.toMatchObject({ + message: "Durable storage is temporarily unavailable", + disposition: "not_sent", + userFacing: true, + }) + }) + it("returns not_sent when no current open socket owns a raw protocol write", async () => { const { result } = renderHook(() => useWebSocket({ url: "ws://localhost", diff --git a/frontend/src/hooks/use-websocket.ts b/frontend/src/hooks/use-websocket.ts index bf31a740e0..0bbef8a7db 100644 --- a/frontend/src/hooks/use-websocket.ts +++ b/frontend/src/hooks/use-websocket.ts @@ -4,6 +4,7 @@ import { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } fr import { useAuth } from "@/contexts/auth-context" import type { AuthSessionSnapshot } from "@/lib/auth-cache" import { apiRequest, getUploadErrorMessage, isJsonRecord, parseApiResponse, UPLOAD_ERROR_MESSAGES } from "@/lib/api-wrapper" +import { isRetriableUploadStatus, uploadOutcomeForStatus, UploadRequestError, withUploadRetry, type UploadOutcome, type UploadRetryOptions } from "@/lib/upload-retry" import { generateClientMessageId, getWsUrl, getUploadApiUrl } from "@/lib/utils" import { isFinalAnswerStreamEventType } from "@/lib/streaming-final-answer" @@ -54,16 +55,39 @@ export type MessageDeliveryDisposition = "not_sent" | "rejected" | "outcome_unkn export class MessageDeliveryError extends Error { readonly disposition: MessageDeliveryDisposition readonly retryWithNewId: boolean + /** + * Whether `message` explains the failure in terms the sender can act on — + * the server's rejection text, or an upload response detail. The remaining + * messages describe connection plumbing ("the connection changed before + * delivery") and are diagnostics: callers show their own localized string + * for those rather than putting internal English in front of a visitor. + */ + readonly userFacing: boolean + /** + * Whether resubmitting the same draft could duplicate a *side effect the + * server cannot deduplicate* - in practice, an attachment that may have + * landed under an id this client never learned. + * + * An unknown *delivery* outcome does not qualify: the turn keeps its + * client message id, so a resubmit lands on the server's existing claim + * instead of opening a second turn. Only an unknown *upload* outcome needs + * a human to reconcile, because uploaded bytes have no such guard. + */ + readonly requiresReconciliation: boolean constructor( message: string, disposition: MessageDeliveryDisposition, retryWithNewId = false, + userFacing = false, + requiresReconciliation = false, ) { super(message) this.name = "MessageDeliveryError" this.disposition = disposition this.retryWithNewId = retryWithNewId + this.userFacing = userFacing + this.requiresReconciliation = requiresReconciliation } } @@ -71,7 +95,106 @@ const deliveryError = ( message: string, disposition: MessageDeliveryDisposition, retryWithNewId = false, -) => new MessageDeliveryError(message, disposition, retryWithNewId) + userFacing = false, + requiresReconciliation = false, +) => new MessageDeliveryError( + message, + disposition, + retryWithNewId, + userFacing, + requiresReconciliation, +) + +type UploadedFileRef = { file_id: string; name?: string; size?: number; type?: string } + +/** + * What an upload attempt ended up doing, decided where the evidence is. + * + * The classification is returned rather than reconstructed from flags by a + * distant catch: every failure exits through one of these two shapes, so a + * case cannot go unclassified by omission. + */ +type UploadStepResult = + | { ok: true; files: UploadedFileRef[] } + | { ok: false; outcome: UploadOutcome; error: Error } + +/** + * Runs one upload - with the shared bounded retry when the caller owns it - + * and reports whether the bytes provably never landed. + * + * `refused` means the server rejected the request and kept nothing, so the + * draft may be sent again as-is. `unknown` means it may have stored the file + * under an id this client never learned: a gateway 5xx, an unreadable body, a + * dropped request, or ownership lost while a request was on the wire. + */ +const runUpload = async ( + perform: () => Promise, + cancellation: Promise, + retry?: UploadRetryOptions, +): Promise => { + // Scoped to this upload, so no later reader can see a stale value. + let inFlight = false + let lastRefusal: UploadOutcome | null = null + const attempt = async () => { + // Each attempt starts having said nothing. Without this, a dropped + // request on attempt two would inherit attempt one's refusal and be + // reported as safe to resend. + lastRefusal = null + inFlight = true + try { + return await perform() + } catch (error) { + if (error instanceof UploadRequestError) lastRefusal = error.outcome + throw error + } finally { + inFlight = false + } + } + + try { + const files = await Promise.race([ + withUploadRetry(attempt, { ...retry, cancellation }), + cancellation, + ]) + return { ok: true, files } + } catch (error) { + const outcome: UploadOutcome = error instanceof UploadRequestError + ? error.outcome + // Not the upload's own error, so ownership was lost. A request still on + // the wire has an unknown fate; a retry only sleeping between attempts + // leaves the server's last word standing. + : inFlight + ? "unknown" + : lastRefusal ?? "unknown" + return { ok: false, outcome, error: error as Error } + } +} + +/** Turns a failed upload into the delivery error the caller should throw. */ +const unwrapUpload = (result: UploadStepResult): UploadedFileRef[] => { + if (result.ok) return result.files + const { error, outcome } = result + const unknown = outcome === "unknown" + if (error instanceof MessageDeliveryError) { + // Ownership loss carries a disposition fixed before the upload's fate was + // known. Keep everything else about it and correct only that. + if (!unknown || error.disposition !== "not_sent") throw error + throw deliveryError( + error.message, + "outcome_unknown", + error.retryWithNewId, + error.userFacing, + true, + ) + } + throw deliveryError( + error.message, + unknown ? "outcome_unknown" : "not_sent", + false, + error instanceof UploadRequestError, + unknown, + ) +} export type WebSocketCredentialOwner = | { @@ -209,6 +332,12 @@ export interface UseWebSocketOptions { token?: string buildWebSocketUrl?: (params: { baseUrl: string; taskId: number; token?: string }) => string uploadFiles?: (files: File[], params: { taskId?: number | null; taskType: string }) => Promise> + /** + * Seam for the upload backoff schedule. Production leaves it unset and gets + * jittered delays; tests pin `sleep`/`random` so a retry's timing is not a + * race they have to win. + */ + uploadRetry?: UploadRetryOptions connection?: WebSocketConnection | null deliveryGeneration?: number onConnectionClose?: (event: CloseEvent) => "handled" | "default" @@ -239,6 +368,7 @@ export function useWebSocket(options: UseWebSocketOptions = {}) { token, buildWebSocketUrl, uploadFiles, + uploadRetry, connection: connectionOption, deliveryGeneration = 0, onConnectionClose, @@ -887,6 +1017,7 @@ export function useWebSocket(options: UseWebSocketOptions = {}) { ? "rejected" : "outcome_unknown", data.retry_with_new_id === true, + typeof data.message === "string" && data.message.trim() !== "", )) } } @@ -1189,35 +1320,72 @@ export function useWebSocket(options: UseWebSocketOptions = {}) { let uploadedFiles: Array<{ file_id: string; name?: string; size?: number; type?: string }> = [] if (filesToUpload.length > 0 && uploadFiles) { - uploadedFiles = await Promise.race([ - uploadFiles(filesToUpload, { - taskId: currentTaskId, - taskType: 'task', - }), + // An injected uploader owns its own retry loop, so this side cannot + // see the gaps between its attempts: the whole call is one attempt. + const attempt = () => uploadFiles(filesToUpload, { + taskId: currentTaskId, + taskType: 'task', + }).then(uploaded => { + if (uploaded.length !== filesToUpload.length) { + // Same 1:1 requirement the default path enforces. No shipped + // transport can return a short list today, but the contract is + // stated nowhere, so assert it rather than deliver a turn whose + // attachments quietly went missing. + throw new UploadRequestError('Upload failed', { + status: null, + retriable: false, + outcome: "unknown", + }) + } + return uploaded + }) + uploadedFiles = unwrapUpload(await runUpload( + attempt, claim.cancellation, - ]) + uploadRetry, + )) } else if (filesToUpload.length > 0) { - const uploadRequest = (async () => { + // One request carries the whole batch, so a retry re-sends every + // file in it. That is only safe for statuses that mean the request + // was refused outright — `isRetriableUploadStatus` draws that line — + // and the 503 the upload endpoint raises for durable storage + // compensates its partial registrations before answering, so the + // retry cannot land duplicates of the files that had staged. + const sendUploadRequest = async () => { const formData = new FormData() filesToUpload.forEach(file => formData.append('files', file)) formData.append('task_type', 'task') formData.append('task_id', currentTaskId.toString()) - const response = await apiRequest(`${getUploadApiUrl()}/api/files/upload`, { - method: 'POST', - headers: { - 'Authorization': `Bearer ${tokenRef.current ?? localStorage.getItem('token') ?? ''}`, + const response = await apiRequest( + `${getUploadApiUrl()}/api/files/upload`, + { + method: 'POST', + headers: { + 'Authorization': `Bearer ${tokenRef.current ?? localStorage.getItem('token') ?? ''}`, + }, + body: formData, }, - body: formData, - }) + // This call owns replay. `apiRequest`'s transport retry would + // re-send a request that may already have committed, and the + // status policy here could then replay the batch on top of it. + { replayTransportFailures: false }, + ) const parsed = await parseApiResponse(response) if (!response.ok || !isJsonRecord(parsed.data)) { - throw deliveryError(getUploadErrorMessage(response, parsed, { - generic: 'Upload failed', - ...UPLOAD_ERROR_MESSAGES, - }), "not_sent") + throw new UploadRequestError( + getUploadErrorMessage(response, parsed, { + generic: 'Upload failed', + ...UPLOAD_ERROR_MESSAGES, + }), + { + status: response.status, + retriable: !response.ok && isRetriableUploadStatus(response.status), + outcome: uploadOutcomeForStatus(response.status), + }, + ) } const data = parsed.data - return data.success && Array.isArray(data.files) + const accepted = data.success && Array.isArray(data.files) ? data.files .filter((file): file is { file_id: string; filename?: string; file_size?: number; mime_type?: string } => ( isJsonRecord(file) && typeof file.file_id === 'string' @@ -1229,9 +1397,41 @@ export function useWebSocket(options: UseWebSocketOptions = {}) { type: typeof file.mime_type === 'string' ? file.mime_type : '', })) : [] - })() - uploadedFiles = await Promise.race([uploadRequest, claim.cancellation]) + if (accepted.length !== filesToUpload.length) { + // The endpoint is all-or-nothing, so a short or unreadable list + // is a response we cannot interpret - not a licence to deliver + // the turn with the attachments quietly missing. + throw new UploadRequestError( + getUploadErrorMessage(response, parsed, { + generic: 'Upload failed', + ...UPLOAD_ERROR_MESSAGES, + }), + { + status: response.status, + retriable: false, + outcome: uploadOutcomeForStatus(response.status), + }, + ) + } + return accepted + } + uploadedFiles = unwrapUpload(await runUpload( + sendUploadRequest, + claim.cancellation, + uploadRetry, + )) } + // Stamp the ids back onto the caller's File objects. A draft that is + // resubmitted after the turn was rejected (or after any later failure) + // then travels as `preUploadedFiles` instead of uploading the same + // bytes again and leaving a duplicate behind. Order is only trusted + // when the server answered for exactly the files that were sent. + if (uploadedFiles.length === filesToUpload.length) { + uploadedFiles.forEach((uploaded, index) => { + if (uploaded.file_id) filesToUpload[index].file_id = uploaded.file_id + }) + } + messageData.files = [...preUploadedFiles, ...uploadedFiles] } @@ -1323,7 +1523,7 @@ export function useWebSocket(options: UseWebSocketOptions = {}) { preparationsRef.current.delete(clientMessageId) } } - }, [isCurrentOwner, uploadFiles]) + }, [isCurrentOwner, uploadFiles, uploadRetry]) const getCurrentTaskConnection = useCallback(() => { const connection = connectionRef.current diff --git a/frontend/src/i18n/locales/en.ts b/frontend/src/i18n/locales/en.ts index 3f5b8c2793..7fa867d8d3 100644 --- a/frontend/src/i18n/locales/en.ts +++ b/frontend/src/i18n/locales/en.ts @@ -315,6 +315,9 @@ const en = { confirmed: "Confirmed", sendFailed: "Failed to send clarification response", sendError: "Failed to send response", + sendNotSent: "Your answers were kept — you can submit again.", + sendOutcomeUnknown: "Your response may already have been submitted. Reload the conversation before submitting again.", + sendDeliveryUnconfirmed: "We could not confirm the response was received. Submitting again is safe — it will not create a second answer.", selectOption: "Select an option", selectOptions: "Select options", acceptedFormats: "Accepted formats", diff --git a/frontend/src/i18n/locales/zh.ts b/frontend/src/i18n/locales/zh.ts index cf257648d7..e3ff8eab61 100644 --- a/frontend/src/i18n/locales/zh.ts +++ b/frontend/src/i18n/locales/zh.ts @@ -315,6 +315,9 @@ const zh = { confirmed: "已确认", sendFailed: "发送澄清回复失败", sendError: "发送回复失败", + sendNotSent: "你填写的内容已保留,可以重新提交。", + sendOutcomeUnknown: "回复可能已经提交,请刷新会话后再重新提交。", + sendDeliveryUnconfirmed: "无法确认回复是否已送达。可以再次提交,不会产生第二条回答。", selectOption: "请选择一个选项", selectOptions: "请选择选项", acceptedFormats: "支持的格式", diff --git a/frontend/src/lib/api-wrapper.test.ts b/frontend/src/lib/api-wrapper.test.ts index a829624117..9f4c214f06 100644 --- a/frontend/src/lib/api-wrapper.test.ts +++ b/frontend/src/lib/api-wrapper.test.ts @@ -849,3 +849,40 @@ describe("api-wrapper auth refresh", () => { } }) }) + +describe("api-wrapper transport replay policy", () => { + const user = { id: "1", username: "alice", email: null, is_admin: false } + + beforeEach(() => { + localStorage.clear() + vi.restoreAllMocks() + mockNavigatorLocks() + }) + + it("replays a dropped request by default", async () => { + writeAuthCache(user, "access", "refresh", 120, 240) + const fetchMock = vi.spyOn(globalThis, "fetch") + .mockRejectedValueOnce(new TypeError("Failed to fetch")) + .mockResolvedValueOnce(new Response(null, { status: 200 })) + + await expect(apiRequest("http://api.local/thing", { method: "POST" })) + .resolves.toMatchObject({ status: 200 }) + expect(fetchMock).toHaveBeenCalledTimes(2) + }) + + it("leaves replay to the caller when it owns the retry policy", async () => { + // A non-idempotent write - file upload - may already have committed when + // the transport drops, so the shared retry must not re-send it underneath + // a caller that decides replay from the response status itself. + writeAuthCache(user, "access", "refresh", 120, 240) + const fetchMock = vi.spyOn(globalThis, "fetch") + .mockRejectedValue(new TypeError("Failed to fetch")) + + await expect(apiRequest( + "http://api.local/api/files/upload", + { method: "POST" }, + { replayTransportFailures: false }, + )).rejects.toThrow("Failed to fetch") + expect(fetchMock).toHaveBeenCalledTimes(1) + }) +}) diff --git a/frontend/src/lib/api-wrapper.ts b/frontend/src/lib/api-wrapper.ts index 01cf808693..dcbe5e4bc1 100644 --- a/frontend/src/lib/api-wrapper.ts +++ b/frontend/src/lib/api-wrapper.ts @@ -143,10 +143,22 @@ function withBearer(options: RequestInit, token: string): RequestInit { return { ...options, headers: { ...options.headers, Authorization: `Bearer ${token}` } } } /** A request has at most one post-401 replay, bound to an exact immutable credential snapshot. */ -export async function apiRequest(url: string, options: RequestInit = {}): Promise { +export interface ApiRequestPolicy { + /** + * Whether a thrown fetch may be replayed by the shared transport retry. + * Callers that own replay themselves - non-idempotent writes such as file + * upload - pass false so a request that may already have committed is not + * silently re-sent underneath them. + */ + replayTransportFailures?: boolean +} +export async function apiRequest(url: string, options: RequestInit = {}, { replayTransportFailures = true }: ApiRequestPolicy = {}): Promise { const session = readAuthSessionSnapshot() if (!session.accessToken) return fetch(url, options) - const response = await fetchWithRetry(url, withBearer(options, session.accessToken)) + const authorized = withBearer(options, session.accessToken) + const response = replayTransportFailures + ? await fetchWithRetry(url, authorized) + : await fetch(url, authorized) if (response.status !== 401 || shouldSkipRefresh(url)) return response const afterResponse = compareAuthSession(session) if (afterResponse.status === "credentials_advanced" || afterResponse.status === "credentials_and_profile_advanced") { diff --git a/frontend/src/lib/public-chat-file-upload.test.ts b/frontend/src/lib/public-chat-file-upload.test.ts index 76fccf634d..28bf1bb48d 100644 --- a/frontend/src/lib/public-chat-file-upload.test.ts +++ b/frontend/src/lib/public-chat-file-upload.test.ts @@ -1,6 +1,8 @@ import { afterEach, describe, expect, it, vi } from "vitest" +import { UPLOAD_ERROR_MESSAGES } from "./api-wrapper" import { uploadPublicChatFile } from "./public-chat-file-upload" +import { UploadRequestError } from "./upload-retry" describe("uploadPublicChatFile", () => { afterEach(() => { @@ -58,3 +60,150 @@ describe("uploadPublicChatFile", () => { }) }) }) + +describe("uploadPublicChatFile transient failures", () => { + afterEach(() => { + vi.restoreAllMocks() + }) + + const jsonResponse = (body: unknown, status: number) => + new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, + }) + + it("retries a 503 from durable storage and keeps the submission alive", async () => { + const fetchMock = vi.spyOn(globalThis, "fetch") + .mockResolvedValueOnce(jsonResponse({ detail: "Storage unavailable" }, 503)) + .mockResolvedValueOnce(jsonResponse({ success: true, file_id: "file-9" }, 200)) + const sleep = vi.fn(async () => {}) + const file = new File(["trip"], "trip.txt", { type: "text/plain" }) + + await expect(uploadPublicChatFile({ + url: "http://api.local/api/widget/files/upload", + accessToken: "guest-token", + file, + taskType: "task", + taskId: 42, + fallbackError: "Upload failed", + retry: { sleep }, + })).resolves.toMatchObject({ file_id: "file-9" }) + + expect(fetchMock).toHaveBeenCalledTimes(2) + expect(sleep).toHaveBeenCalledTimes(1) + }) + + it("surfaces the storage failure with its status once retries are exhausted", async () => { + // A fresh Response per attempt: a body can only be read once. + vi.spyOn(globalThis, "fetch").mockImplementation( + async () => jsonResponse({ detail: "Storage unavailable" }, 503), + ) + const sleep = vi.fn(async () => {}) + const file = new File(["trip"], "trip.txt", { type: "text/plain" }) + + const failure = await uploadPublicChatFile({ + url: "http://api.local/api/widget/files/upload", + accessToken: "guest-token", + file, + taskType: "task", + fallbackError: "Upload failed", + retry: { sleep }, + }).catch((error: unknown) => error) + + expect(failure).toBeInstanceOf(UploadRequestError) + expect(failure).toMatchObject({ status: 503, retriable: true }) + expect((failure as Error).message).toBe("Storage unavailable") + }) + + it("does not retry a permanent rejection", async () => { + const fetchMock = vi.spyOn(globalThis, "fetch").mockResolvedValue( + jsonResponse({ detail: "Unsupported file type" }, 400), + ) + const file = new File(["trip"], "trip.txt", { type: "text/plain" }) + + await expect(uploadPublicChatFile({ + url: "http://api.local/api/widget/files/upload", + accessToken: "guest-token", + file, + taskType: "task", + fallbackError: "Upload failed", + })).rejects.toThrow("Unsupported file type") + + expect(fetchMock).toHaveBeenCalledTimes(1) + }) + + it("reports a gateway HTML failure instead of the generic fallback", async () => { + vi.spyOn(globalThis, "fetch").mockImplementation( + async () => new Response("503 Service Unavailable", { + status: 503, + headers: { "Content-Type": "text/html" }, + }), + ) + const sleep = vi.fn(async () => {}) + const file = new File(["trip"], "trip.txt", { type: "text/plain" }) + + await expect(uploadPublicChatFile({ + url: "http://api.local/api/widget/files/upload", + accessToken: "guest-token", + file, + taskType: "task", + fallbackError: "Upload failed", + retry: { sleep }, + })).rejects.toThrow(UPLOAD_ERROR_MESSAGES.proxy) + }) +}) + +describe("uploadPublicChatFile partial batches", () => { + afterEach(() => { + vi.restoreAllMocks() + }) + + const upload = (file: File) => uploadPublicChatFile({ + url: "http://api.local/api/widget/files/upload", + accessToken: "guest-token", + file, + taskType: "task", + fallbackError: "Upload failed", + }) + + it("keeps a completed file's id when a sibling in the same batch fails", async () => { + // The widget transport uploads one request per file under Promise.all, so + // a sibling's rejection must not lose what already landed. + vi.spyOn(globalThis, "fetch").mockImplementation(async (_url, request) => { + const sent = (request?.body as FormData).get("file") as File + return sent.name === "good.txt" + ? new Response(JSON.stringify({ success: true, file_id: "file-good" }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }) + : new Response(JSON.stringify({ detail: "File is too large" }), { + status: 413, + headers: { "Content-Type": "application/json" }, + }) + }) + const good = new File(["ok"], "good.txt") as File & { file_id?: string } + const bad = new File(["nope"], "bad.txt") as File & { file_id?: string } + + await expect(Promise.all([upload(good), upload(bad)])) + .rejects.toThrow("File is too large") + + expect(good.file_id).toBe("file-good") + expect(bad.file_id).toBeUndefined() + }) + + it("reports an unreadable success body as a possibly stored upload", async () => { + vi.spyOn(globalThis, "fetch").mockImplementation( + async () => new Response(JSON.stringify({ success: true }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }), + ) + const file = new File(["ok"], "good.txt") as File & { file_id?: string } + + const failure = await upload(file).catch((error: unknown) => error) + + expect(failure).toBeInstanceOf(UploadRequestError) + expect(failure).toMatchObject({ outcome: "unknown" }) + expect(file.file_id).toBeUndefined() + }) +}) diff --git a/frontend/src/lib/public-chat-file-upload.ts b/frontend/src/lib/public-chat-file-upload.ts index e788abd8f2..d9302a7905 100644 --- a/frontend/src/lib/public-chat-file-upload.ts +++ b/frontend/src/lib/public-chat-file-upload.ts @@ -1,3 +1,17 @@ +import { + getUploadErrorMessage, + isJsonRecord, + parseApiResponse, + UPLOAD_ERROR_MESSAGES, +} from "@/lib/api-wrapper" +import { + isRetriableUploadStatus, + uploadOutcomeForStatus, + UploadRequestError, + withUploadRetry, + type UploadRetryOptions, +} from "@/lib/upload-retry" + export interface PublicChatUploadedFile { file_id: string name?: string @@ -12,15 +26,19 @@ interface UploadPublicChatFileOptions { taskType: string taskId?: number | string | null fallbackError: string + retry?: UploadRetryOptions } -interface PublicChatUploadResponse { - success?: boolean - file_id?: unknown - detail?: unknown - message?: unknown -} - +/** + * Uploads one file for the widget/share chat. + * + * Deliberately uses `fetch` rather than `apiRequest`: public visitors carry a + * guest token, so the shared 401 handling (which redirects to /login) does not + * apply here. + * + * One request carries exactly one file, so a bounded retry of a refused + * request cannot duplicate a sibling file that already landed. + */ export async function uploadPublicChatFile({ url, accessToken, @@ -28,35 +46,53 @@ export async function uploadPublicChatFile({ taskType, taskId, fallbackError, + retry, }: UploadPublicChatFileOptions): Promise { - const formData = new FormData() - formData.append("file", file) - formData.append("task_type", taskType) - if (taskId != null) { - formData.append("task_id", taskId.toString()) - } + const sendUpload = async (): Promise => { + const formData = new FormData() + formData.append("file", file) + formData.append("task_type", taskType) + if (taskId != null) { + formData.append("task_id", taskId.toString()) + } - const response = await fetch(url, { - method: "POST", - headers: { "Authorization": `Bearer ${accessToken}` }, - body: formData, - }) - const data = await response.json().catch(() => null) as PublicChatUploadResponse | null - const fileId = typeof data?.file_id === "string" ? data.file_id : null + const response = await fetch(url, { + method: "POST", + headers: { "Authorization": `Bearer ${accessToken}` }, + body: formData, + }) + const parsed = await parseApiResponse(response) + const data = isJsonRecord(parsed.data) ? parsed.data : null + const fileId = typeof data?.file_id === "string" ? data.file_id : null - if (!response.ok || data?.success !== true || !fileId) { - const backendMessage = typeof data?.detail === "string" - ? data.detail - : typeof data?.message === "string" - ? data.message - : null - throw new Error(backendMessage || fallbackError) - } + if (!response.ok || data?.success !== true || !fileId) { + throw new UploadRequestError( + getUploadErrorMessage(response, parsed, { + generic: fallbackError, + ...UPLOAD_ERROR_MESSAGES, + }), + { + status: response.status, + retriable: !response.ok && isRetriableUploadStatus(response.status), + // An unreadable success body is the ambiguous case: the file may be + // stored under an id this client never learned. + outcome: uploadOutcomeForStatus(response.status), + }, + ) + } - return { - file_id: fileId, - name: file.name, - size: file.size, - type: file.type, + return { + file_id: fileId, + name: file.name, + size: file.size, + type: file.type, + } } + + const uploaded = await withUploadRetry(sendUpload, retry) + // Stamp the id onto this file as soon as it lands, not after the caller's + // `Promise.all` settles: a sibling's failure rejects the aggregate, and a + // draft resubmitted without this would upload these bytes a second time. + ;(file as File & { file_id?: string }).file_id = uploaded.file_id + return uploaded } diff --git a/frontend/src/lib/upload-retry.test.ts b/frontend/src/lib/upload-retry.test.ts new file mode 100644 index 0000000000..a226cbe017 --- /dev/null +++ b/frontend/src/lib/upload-retry.test.ts @@ -0,0 +1,135 @@ +import { describe, expect, it, vi } from "vitest" + +import { + isRetriableUploadStatus, + uploadOutcomeForStatus, + UploadRequestError, + withUploadRetry, +} from "./upload-retry" + +const retriableError = (status: number) => + new UploadRequestError("Storage unavailable", { + status, + retriable: isRetriableUploadStatus(status), + outcome: uploadOutcomeForStatus(status), + }) + +describe("withUploadRetry", () => { + it("retries a 503 and returns the eventual success", async () => { + const sleep = vi.fn(async () => {}) + const perform = vi.fn() + .mockRejectedValueOnce(retriableError(503)) + .mockResolvedValueOnce("uploaded") + + await expect(withUploadRetry(perform, { sleep, random: () => 1 })) + .resolves.toBe("uploaded") + expect(perform).toHaveBeenCalledTimes(2) + expect(sleep).toHaveBeenCalledWith(400) + }) + + it("backs off exponentially and gives up with the last error", async () => { + const sleep = vi.fn<(ms: number) => Promise>(async () => {}) + const perform = vi.fn().mockRejectedValue(retriableError(503)) + + await expect(withUploadRetry(perform, { sleep, random: () => 1 })) + .rejects.toThrow("Storage unavailable") + expect(perform).toHaveBeenCalledTimes(3) + expect(sleep.mock.calls.map(([ms]) => ms)).toEqual([400, 800]) + }) + + it("spreads retries with full jitter so a fleet does not march in lockstep", async () => { + const sleep = vi.fn<(ms: number) => Promise>(async () => {}) + const perform = vi.fn().mockRejectedValue(retriableError(503)) + + await expect(withUploadRetry(perform, { sleep, random: () => 0.25 })) + .rejects.toThrow("Storage unavailable") + expect(sleep.mock.calls.map(([ms]) => ms)).toEqual([100, 200]) + }) + + it("does not retry a client-side rejection", async () => { + const sleep = vi.fn(async () => {}) + const perform = vi.fn().mockRejectedValue(retriableError(413)) + + await expect(withUploadRetry(perform, { sleep })).rejects.toThrow( + "Storage unavailable", + ) + expect(perform).toHaveBeenCalledTimes(1) + expect(sleep).not.toHaveBeenCalled() + }) + + it.each([502, 504])( + "does not retry a %i, whose outcome is unknown", + async (status) => { + // Both mean a proxy reached the upstream and could not get a usable + // answer back, so the upload may already have landed. + const sleep = vi.fn(async () => {}) + const perform = vi.fn().mockRejectedValue(retriableError(status)) + + await expect(withUploadRetry(perform, { sleep })).rejects.toThrow( + "Storage unavailable", + ) + expect(perform).toHaveBeenCalledTimes(1) + expect(sleep).not.toHaveBeenCalled() + }, + ) + + it("does not retry a rejected request whose outcome is unknown", async () => { + const sleep = vi.fn(async () => {}) + // A dropped connection surfaces as a plain TypeError from fetch: the + // server may already have stored the file, so a retry could duplicate it. + const perform = vi.fn().mockRejectedValue(new TypeError("Failed to fetch")) + + await expect(withUploadRetry(perform, { sleep })).rejects.toThrow( + "Failed to fetch", + ) + expect(perform).toHaveBeenCalledTimes(1) + expect(sleep).not.toHaveBeenCalled() + }) + + it("waits on a real timer when no sleep is injected", async () => { + vi.useFakeTimers() + try { + const perform = vi.fn() + .mockRejectedValueOnce(retriableError(503)) + .mockResolvedValueOnce("uploaded") + const upload = withUploadRetry(perform) + + await vi.advanceTimersByTimeAsync(400) + + await expect(upload).resolves.toBe("uploaded") + expect(perform).toHaveBeenCalledTimes(2) + } finally { + vi.useRealTimers() + } + }) + + it("abandons the backoff wait once the caller loses ownership", async () => { + const cancellation = Promise.reject(new Error("connection changed")) + cancellation.catch(() => {}) + const perform = vi.fn().mockRejectedValue(retriableError(503)) + + await expect(withUploadRetry(perform, { + cancellation, + sleep: () => new Promise(() => {}), + })).rejects.toThrow("connection changed") + expect(perform).toHaveBeenCalledTimes(1) + }) +}) + +describe("uploadOutcomeForStatus", () => { + it("treats a refusal as proof that nothing was stored", () => { + // 4xx is the server declining the request; 503 is the upload endpoint + // answering only after it rolled back what it had staged. + for (const status of [400, 401, 413, 422, 429, 503]) { + expect(uploadOutcomeForStatus(status)).toBe("refused") + } + }) + + it("treats anything else as possibly stored", () => { + // A proxy reached the upstream (502/504), the server failed after + // committing (500), or the success body could not be read (200). + for (const status of [200, 500, 502, 504]) { + expect(uploadOutcomeForStatus(status)).toBe("unknown") + } + }) +}) diff --git a/frontend/src/lib/upload-retry.ts b/frontend/src/lib/upload-retry.ts new file mode 100644 index 0000000000..9e2e74d6c0 --- /dev/null +++ b/frontend/src/lib/upload-retry.ts @@ -0,0 +1,109 @@ +/** + * Bounded retry for file uploads. + * + * Only failures that carry a definite retriable HTTP status are retried: a + * rejected `fetch` (connection dropped, request aborted mid-body) leaves the + * outcome unknown, and the server may already have persisted the upload, so + * retrying it would duplicate the file. Callers therefore have to raise an + * `UploadRequestError` — anything else propagates on the first attempt. + */ + +/** + * Statuses that mean the request was *refused* before anything was stored, so + * re-sending it cannot duplicate the file. + * + * Only 503 qualifies. The upload endpoint raises it for durable storage after + * compensating whatever it had staged, and a gateway raises it when it has no + * upstream to hand the request to; either way nothing was retained. + * + * 502 and 504 are excluded for the same reason: both mean a proxy did reach + * the upstream and then failed to get a usable answer out of it, so the upload + * may well have landed. Widening this set past "provably refused" needs a + * server-side idempotency key, not a bigger status list. + */ +const RETRIABLE_UPLOAD_STATUSES = new Set([503]) + +export function isRetriableUploadStatus(status: number): boolean { + return RETRIABLE_UPLOAD_STATUSES.has(status) +} + +/** + * Whether a failed upload provably stored nothing. + * + * `refused` means the server rejected the request outright, so the caller may + * offer a plain retry. `unknown` means the bytes may have landed without the + * client learning the id - a gateway 5xx, or a success body it could not + * read - and resubmitting the same draft could duplicate the file. + */ +export type UploadOutcome = "refused" | "unknown" + +export function uploadOutcomeForStatus(status: number): UploadOutcome { + if (status >= 400 && status < 500) return "refused" + return isRetriableUploadStatus(status) ? "refused" : "unknown" +} + +export class UploadRequestError extends Error { + readonly status: number | null + readonly retriable: boolean + readonly outcome: UploadOutcome + + constructor( + message: string, + { + status, + retriable, + outcome, + }: { status: number | null; retriable: boolean; outcome: UploadOutcome }, + ) { + super(message) + this.name = "UploadRequestError" + this.status = status + this.retriable = retriable + this.outcome = outcome + } +} + +/** Total attempts, including the first one. */ +const MAX_ATTEMPTS = 3 +/** Ceiling for the wait before the second attempt; doubles after that. */ +const BASE_DELAY_MS = 400 + +export interface UploadRetryOptions { + /** + * Rejects when the caller no longer owns the upload (connection swapped, + * message superseded). Aborts the backoff wait instead of letting a dead + * submission keep retrying. + */ + cancellation?: Promise + sleep?: (ms: number) => Promise + /** Injectable for tests; returns [0, 1). */ + random?: () => number +} + +const defaultSleep = (ms: number) => + new Promise(resolve => { setTimeout(resolve, ms) }) + +export async function withUploadRetry( + perform: () => Promise, + { + cancellation, + sleep = defaultSleep, + random = Math.random, + }: UploadRetryOptions = {}, +): Promise { + for (let attempt = 1; ; attempt += 1) { + try { + return await perform() + } catch (error) { + const retriable = + error instanceof UploadRequestError && error.retriable + if (!retriable || attempt >= MAX_ATTEMPTS) throw error + // Full jitter. A storage outage fails every in-flight upload at once, + // and the widget fans out one request per file, so a fixed schedule + // would march the whole fleet back onto the endpoint in lockstep. + const delay = Math.round(random() * BASE_DELAY_MS * 2 ** (attempt - 1)) + const wait = sleep(delay) + await (cancellation ? Promise.race([wait, cancellation]) : wait) + } + } +} diff --git a/frontend/vitest.widget.config.ts b/frontend/vitest.widget.config.ts index f4e588157e..fa995bde10 100644 --- a/frontend/vitest.widget.config.ts +++ b/frontend/vitest.widget.config.ts @@ -10,6 +10,7 @@ const widgetConfig = mergeConfig(baseConfig, defineConfig({ "src/app/widget/chat/[[]token[]]/page-client.tsx", "src/components/chat/ChatInput.tsx", "src/components/chat/ChatMessage.tsx", + "src/components/chat/clarification-form.tsx", "src/components/chat/TraceEventRenderer.tsx", "src/components/file/file-preview-content.tsx", "src/components/file/file-viewer.tsx", @@ -28,6 +29,8 @@ const widgetConfig = mergeConfig(baseConfig, defineConfig({ "src/lib/api-wrapper.ts", "src/lib/auth-cache.ts", "src/lib/files-disabled-presentation.ts", + "src/lib/public-chat-file-upload.ts", + "src/lib/upload-retry.ts", "src/contexts/presentation-capabilities.tsx", "src/app/settings/page.tsx", "src/components/layout/sidebar.tsx", @@ -65,6 +68,12 @@ const widgetConfig = mergeConfig(baseConfig, defineConfig({ "src/lib/files-disabled-presentation.ts": { statements: 85, branches: 80, functions: 90, lines: 85, }, + "src/lib/public-chat-file-upload.ts": { + statements: 95, branches: 90, functions: 100, lines: 95, + }, + "src/lib/upload-retry.ts": { + statements: 95, branches: 90, functions: 90, lines: 95, + }, "src/lib/auth-cache.ts": { statements: 90, branches: 80, functions: 90, lines: 90 }, "src/contexts/presentation-capabilities.tsx": { statements: 100, branches: 100, functions: 100, lines: 100, @@ -88,6 +97,9 @@ const widgetConfig = mergeConfig(baseConfig, defineConfig({ "src/components/pages/oidc-callback.tsx": { statements: 75, branches: 45, functions: 95, lines: 75 }, "src/components/chat/ChatInput.tsx": { statements: 60, branches: 60, functions: 40, lines: 60 }, "src/components/chat/ChatMessage.tsx": { statements: 50, branches: 50, functions: 40, lines: 50 }, + "src/components/chat/clarification-form.tsx": { + statements: 75, branches: 65, functions: 60, lines: 75, + }, "src/components/chat/TraceEventRenderer.tsx": { statements: 80, branches: 75, functions: 75, lines: 80, }, @@ -152,6 +164,8 @@ export default defineConfig({ "src/lib/api-wrapper.test.ts", "src/lib/auth-cache.test.ts", "src/lib/files-disabled-presentation.test.ts", + "src/lib/public-chat-file-upload.test.ts", + "src/lib/upload-retry.test.ts", ], }, }) diff --git a/src/xagent/web/api/files.py b/src/xagent/web/api/files.py index 9e6ce3ef96..415384966d 100644 --- a/src/xagent/web/api/files.py +++ b/src/xagent/web/api/files.py @@ -114,12 +114,33 @@ def _durable_storage_unavailable() -> HTTPException: + """The upload was refused and everything it staged has been rolled back. + + Clients treat 503 from this endpoint as proof that no side effect + survived, and retry on it. Only raise it once compensation has actually + succeeded - see ``_durable_storage_outcome_unknown`` for the other case. + """ return HTTPException( status_code=503, detail="Durable storage is temporarily unavailable", ) +def _durable_storage_outcome_unknown() -> HTTPException: + """Storage failed and the rollback of what it staged also failed. + + Rows or objects may survive, so this must not be a status the client + replays; a retry would duplicate whatever the compensation left behind. + """ + return HTTPException( + status_code=500, + detail=( + "The upload could not be completed or rolled back. " + "Refresh the conversation before trying again." + ), + ) + + def _file_integrity_failed() -> HTTPException: return HTTPException( status_code=409, @@ -477,6 +498,8 @@ async def store_uploaded_files( registrations: list[LocalUploadRegistration] = [] previews: dict[str, Any] = {} completed = False + durable_storage_refused = False + registrations_rolled_back = False try: # Off-turn (this runs outside the ExecutionScopeContext/ @@ -588,12 +611,14 @@ async def store_uploaded_files( ) completed = True except DurableStorageOperationError as exc: + durable_storage_refused = True logger.warning("Durable storage unavailable during upload: %s", exc) raise _durable_storage_unavailable() from exc finally: if not completed: async def _cleanup() -> None: + nonlocal registrations_rolled_back try: await run_db_io_cancellation_safe( lambda: compensate_registered_uploads_sync( @@ -603,6 +628,7 @@ async def _cleanup() -> None: ) ) ) + registrations_rolled_back = True finally: def _delete_local_paths() -> None: @@ -622,6 +648,18 @@ def _delete_local_paths() -> None: except Exception: logger.exception("Failed to compensate cancelled/failed upload") + # 503 is the client's licence to replay the whole batch, so it may + # only escape when the rollback actually succeeded. Downgrading a + # failed rollback to an unknown outcome keeps a retry from + # duplicating whatever survived. Other in-flight failures keep + # their own status: they are not statuses the client replays. + # + # Only the *registration* rollback decides this. A staged temp file + # that could not be unlinked leaves no row for a retry to + # duplicate, so it must not cost the client its replay. + if durable_storage_refused and not registrations_rolled_back: + raise _durable_storage_outcome_unknown() + if single_file_mode: first_file = uploaded_files[0] return { diff --git a/tests/web/api/test_upload_connection_boundary.py b/tests/web/api/test_upload_connection_boundary.py index cb2cb00d0e..fdd2ce8465 100644 --- a/tests/web/api/test_upload_connection_boundary.py +++ b/tests/web/api/test_upload_connection_boundary.py @@ -986,3 +986,164 @@ def counting_put_file( ) assert Path(str(file_info["workspace_path"])).exists() assert tmp_path.exists() + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("compensation_fails", "expected_status", "expected_detail"), + [ + (False, 503, "Durable storage is temporarily unavailable"), + ( + True, + 500, + "The upload could not be completed or rolled back. " + "Refresh the conversation before trying again.", + ), + ], + ids=["compensated", "compensation-failed"], +) +async def test_durable_storage_503_only_escapes_when_rollback_succeeded( + monkeypatch: pytest.MonkeyPatch, + isolated_upload_storage, + compensation_fails: bool, + expected_status: int, + expected_detail: str, +) -> None: + """Clients replay 503, so it may only mean "nothing was retained". + + A failed rollback can leave rows or objects behind; answering 503 there + would invite a retry that duplicates them (PR #1472 review finding N1). + """ + _upload_root, _object_root = isolated_upload_storage + _admin_headers() + db = _direct_db_session() + try: + user_id = int(db.query(User.id).filter(User.username == "admin").scalar()) + finally: + db.close() + + def fail_registration(_registrations) -> None: # type: ignore[no-untyped-def] + raise DurableStorageOperationError("durable write unavailable") + + def compensate(_claims) -> None: # type: ignore[no-untyped-def] + if compensation_fails: + raise DurableStorageOperationError("storage cleanup unavailable") + + monkeypatch.setattr(files_api, "register_local_uploads_sync", fail_registration) + monkeypatch.setattr(files_api, "compensate_registered_uploads_sync", compensate) + + with pytest.raises(HTTPException) as raised: + await files_api.store_uploaded_files( + upload_items=[ + UploadFile( + filename="durable-storage-outcome.txt", + file=io.BytesIO(b"payload"), + headers={"content-type": "text/plain"}, + ) + ], + task_type="general", + task_id=None, + folder=None, + user_id=user_id, + single_file_mode=True, + ) + + assert raised.value.status_code == expected_status + # The client shows this detail verbatim, so it is part of the contract. + assert raised.value.detail == expected_detail + + +@pytest.mark.asyncio +async def test_failed_compensation_alone_does_not_rewrite_another_failure( + monkeypatch: pytest.MonkeyPatch, + isolated_upload_storage, +) -> None: + """The downgrade is for the replayable status only. + + A failure the client never replays keeps its own error even when the + rollback also fails - otherwise every failed cleanup would masquerade as + an unknown *upload* outcome. + """ + _upload_root, _object_root = isolated_upload_storage + _admin_headers() + db = _direct_db_session() + try: + user_id = int(db.query(User.id).filter(User.username == "admin").scalar()) + finally: + db.close() + + def fail_registration(_registrations) -> None: # type: ignore[no-untyped-def] + raise RuntimeError("registration failed") + + def fail_compensation(_claims) -> None: # type: ignore[no-untyped-def] + raise DurableStorageOperationError("storage cleanup unavailable") + + monkeypatch.setattr(files_api, "register_local_uploads_sync", fail_registration) + monkeypatch.setattr( + files_api, "compensate_registered_uploads_sync", fail_compensation + ) + + with pytest.raises(RuntimeError, match="registration failed"): + await files_api.store_uploaded_files( + upload_items=[ + UploadFile( + filename="unrelated-failure.txt", + file=io.BytesIO(b"payload"), + headers={"content-type": "text/plain"}, + ) + ], + task_type="general", + task_id=None, + folder=None, + user_id=user_id, + single_file_mode=True, + ) + + +@pytest.mark.asyncio +async def test_unlinkable_staged_file_does_not_cost_the_client_its_replay( + monkeypatch: pytest.MonkeyPatch, + isolated_upload_storage, +) -> None: + """A staged temp file leaves no row, so failing to unlink it is not doubt. + + Only the registration rollback decides whether 503 may escape; conflating + the two would tell a visitor to reload over an orphaned temp file. + """ + _upload_root, _object_root = isolated_upload_storage + _admin_headers() + db = _direct_db_session() + try: + user_id = int(db.query(User.id).filter(User.username == "admin").scalar()) + finally: + db.close() + + def fail_registration(_registrations) -> None: # type: ignore[no-untyped-def] + raise DurableStorageOperationError("durable write unavailable") + + def fail_delete(_path) -> None: # type: ignore[no-untyped-def] + raise OSError("device busy") + + monkeypatch.setattr(files_api, "register_local_uploads_sync", fail_registration) + monkeypatch.setattr( + files_api, "compensate_registered_uploads_sync", lambda _claims: None + ) + monkeypatch.setattr(files_api, "_delete_staged_upload", fail_delete) + + with pytest.raises(HTTPException) as raised: + await files_api.store_uploaded_files( + upload_items=[ + UploadFile( + filename="unlinkable.txt", + file=io.BytesIO(b"payload"), + headers={"content-type": "text/plain"}, + ) + ], + task_type="general", + task_id=None, + folder=None, + user_id=user_id, + single_file_mode=True, + ) + + assert raised.value.status_code == 503