Display API error messages during image upload (#3227)

Closes #1859. Will merge this into main and then merge main into #3222.

<img width="773" height="442" alt="image"
src="https://github.com/user-attachments/assets/2443b3e6-a994-4b2a-8491-3cabc134e1bd"
/>

Author: david-crespo

app/api/__tests__/errors.spec.ts

@@ -42,7 +42,6 @@ describe('processServerError', () => {
       },
     }
     expect(processServerError('fakeThingView', parseError)).toEqual({
-      code: undefined,
       message: 'Hi, you have an error',
       statusCode: 400,
       errorCode: undefined,
@@ -60,6 +59,7 @@ describe('processServerError', () => {
     expect(processServerError('fakeThingView', clientError)).toEqual({
       message: 'Error reading API response',
       statusCode: 200,
+      requestId: undefined,
     })
   })
 

app/components/form/SideModalForm.tsx

@@ -9,8 +9,6 @@
 import { useEffect, useId, useState, type ReactNode } from 'react'
 import type { FieldValues, UseFormReturn } from 'react-hook-form'
 
-import type { ApiError } from '@oxide/api'
-
 import { useShouldAnimateModal } from '~/hooks/use-should-animate-modal'
 import { Button } from '~/ui/lib/Button'
 import { Modal } from '~/ui/lib/Modal'
@@ -39,8 +37,10 @@ type SideModalFormProps<TFieldValues extends FieldValues> = {
   // require loading and error so we can't forget to hook them up. there are a
   // few forms that don't need them, so we'll use dummy values
 
-  /** Error from the API call */
-  submitError: ApiError | null
+  // would be ApiError except image-upload synthesizes a local "name already
+  // exists" error that isn't from the API. The modal only reads message and
+  // errorCode anyway.
+  submitError: { message: string; errorCode?: string } | null
   loading: boolean
 
   /** Only needed if you need to override the default title (Create/Edit ${resourceName}) */

app/forms/image-upload.tsx

@@ -18,7 +18,6 @@ import {
   q,
   queryClient,
   useApiMutation,
-  type ApiError,
   type BlockSize,
   type Disk,
   type Snapshot,
@@ -54,6 +53,24 @@ import { formatBytes, GiB, KiB } from '~/util/units'
 // Padded because otherwise the numbers jump around a bit, e.g., when it goes
 // from 10.55 to 14.7 to 19.23
 const fsize = (bytes: number) => formatBytes(bytes, { pad: true }).label
+const genericUploadErrorMessage = 'Something went wrong. Please try again.'
+
+function getUploadErrorMessage(e: unknown): string {
+  if (!e || typeof e !== 'object') return genericUploadErrorMessage
+  const errorCode = 'errorCode' in e ? e.errorCode : undefined
+  const message = 'message' in e ? e.message : undefined
+
+  // Mutation errors are ApiErrors with user-facing messages from
+  // processServerError. Show the API message only when it's actually
+  // user-facing: errorCode "Internal" maps to dropshot's generic 500
+  // ("Internal Server Error") with no real detail, and a missing message
+  // means we got nothing to display.
+  return typeof errorCode === 'string' &&
+    typeof message === 'string' &&
+    errorCode !== 'Internal'
+    ? message
+    : genericUploadErrorMessage
+}
 
 type FormValues = {
   imageName: string
@@ -132,6 +149,7 @@ function getTmpDiskName(imageName: string) {
     // do the right thing in
     const specialNames = new Set([
       'disk-create-500',
+      'disk-create-quota',
       'import-start-500',
       'import-stop-500',
       'disk-finalize-500',
@@ -195,7 +213,7 @@ export default function ImageCreate() {
   // are submitting, we rely on the RQ mutations themselves, plus a synthetic
   // mutation state representing the many calls of the bulk upload step.
 
-  const [formError, setFormError] = useState<ApiError | null>(null)
+  const [formError, setFormError] = useState<{ message: string } | null>(null)
   const [modalOpen, setModalOpen] = useState(false)
   const [modalError, setModalError] = useState<string | null>(null)
 
@@ -536,10 +554,7 @@ export default function ImageCreate() {
         if (image) {
           // TODO: set this error on the field instead of the whole form
           // TODO: make setError available here somehow :(
-          setFormError({
-            errorCode: 'ObjectAlreadyExists',
-            message: 'Image name already exists',
-          })
+          setFormError({ message: 'Image name already exists' })
           return
         }
 
@@ -552,7 +567,7 @@ export default function ImageCreate() {
         } catch (e) {
           if (e !== ABORT_ERROR) {
             console.error(e)
-            setModalError('Something went wrong. Please try again.')
+            setModalError(getUploadErrorMessage(e))
             // abort anything in flight in case
             cancelEverything()
           }

mock-api/msw/handlers.ts

@@ -55,6 +55,7 @@ import {
   getBlockSize,
   handleMetrics,
   handleOxqlMetrics,
+  internalError,
   invalidRequest,
   ipRangeLen,
   NotImplemented,
@@ -157,7 +158,22 @@ export const handlers = makeHandlers({
 
     errIfExists(db.disks, { name: body.name, project_id: project.id })
 
-    if (body.name === 'disk-create-500') throw 500
+    if (body.name === 'disk-create-500') throw internalError('disk create failed')
+
+    // Mirrors the InsufficientCapacity error from omicron's virtual
+    // provisioning collection update when a project's storage quota is
+    // exceeded. See NOT_ENOUGH_STORAGE_SENTINEL in
+    // https://github.com/oxidecomputer/omicron/blob/aa608203/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs#L61-L67
+    if (body.name === 'disk-create-quota') {
+      throw json(
+        {
+          error_code: 'InsufficientCapacity',
+          message:
+            'Storage Limit Exceeded: Not enough storage to complete request. Either remove unneeded disks and snapshots to free up resources or contact the rack operator to request a capacity increase.',
+        },
+        { status: 507 }
+      )
+    }
 
     const { name, description, size, disk_backend } = body
     const diskSource = disk_backend.type === 'distributed' ? disk_backend.disk_source : null
@@ -220,7 +236,7 @@ export const handlers = makeHandlers({
   async diskBulkWriteImportStart({ path, query }) {
     const disk = lookup.disk({ ...path, ...query })
 
-    if (disk.name === 'import-start-500') throw 500
+    if (disk.name === 'import-start-500') throw internalError('import start failed')
 
     if (disk.state.state !== 'import_ready') {
       throw 'Can only enter state importing_from_bulk_write from import_ready'
@@ -235,7 +251,7 @@ export const handlers = makeHandlers({
   async diskBulkWriteImportStop({ path, query }) {
     const disk = lookup.disk({ ...path, ...query })
 
-    if (disk.name === 'import-stop-500') throw 500
+    if (disk.name === 'import-stop-500') throw internalError('import stop failed')
 
     if (disk.state.state !== 'importing_from_bulk_writes') {
       throw 'Can only stop import for disk in state importing_from_bulk_write'
@@ -258,7 +274,7 @@ export const handlers = makeHandlers({
   diskFinalizeImport: ({ path, query, body }) => {
     const disk = lookup.disk({ ...path, ...query })
 
-    if (disk.name === 'disk-finalize-500') throw 500
+    if (disk.name === 'disk-finalize-500') throw internalError('disk finalize failed')
 
     if (disk.state.state !== 'import_ready') {
       throw `Cannot finalize disk in state ${disk.state.state}. Must be import_ready.`

mock-api/msw/util.ts

@@ -113,8 +113,16 @@ export const NotImplemented = () => {
 export const invalidRequest = (message: string) =>
   json({ error_code: 'InvalidRequest', message }, { status: 400 })
 
-export const internalError = (message: string) =>
-  json({ error_code: 'InternalError', message }, { status: 500 })
+// 500s in Omicron come from  Error::InternalError, which turns into dropshot's
+// `for_internal_error`, which sets error_code "Internal" and a external message
+// of "Internal Server Error". It also has an `internal_message` that gets
+// logged but isn't sent to clients. We imitate that here.
+// https://github.com/oxidecomputer/omicron/blob/985304a/common/src/api/external/error.rs#L474-L476
+// https://github.com/oxidecomputer/dropshot/blob/9b431d1/dropshot/src/error.rs#L229-L241
+export const internalError = (internalMessage?: string) => {
+  if (internalMessage) console.error(internalMessage)
+  return json({ error_code: 'Internal', message: 'Internal Server Error' }, { status: 500 })
+}
 
 export const errIfExists = <T extends Record<string, unknown>>(
   collection: T[],

test/e2e/image-upload.e2e.ts

@@ -262,14 +262,39 @@ test.describe('Image upload', () => {
     await expectUploadProcess(page)
   })
 
+  // generic 500s (errorCode "Internal") have no useful API message, so we fall
+  // back to a generic one; specific error codes like InsufficientCapacity carry
+  // a real user-facing message that we surface verbatim.
+  const genericMessage = 'Something went wrong. Please try again.'
   const failureCases = [
-    { imageName: 'disk-create-500', stepText: 'Create temporary disk' },
-    { imageName: 'import-start-500', stepText: 'Put disk in import mode' },
-    { imageName: 'import-stop-500', stepText: 'Get disk out of import mode' },
-    { imageName: 'disk-finalize-500', stepText: 'Finalize disk and create snapshot' },
+    {
+      imageName: 'disk-create-500',
+      stepText: 'Create temporary disk',
+      message: genericMessage,
+    },
+    {
+      imageName: 'disk-create-quota',
+      stepText: 'Create temporary disk',
+      message: 'Storage Limit Exceeded',
+    },
+    {
+      imageName: 'import-start-500',
+      stepText: 'Put disk in import mode',
+      message: genericMessage,
+    },
+    {
+      imageName: 'import-stop-500',
+      stepText: 'Get disk out of import mode',
+      message: genericMessage,
+    },
+    {
+      imageName: 'disk-finalize-500',
+      stepText: 'Finalize disk and create snapshot',
+      message: genericMessage,
+    },
   ]
 
-  for (const { imageName, stepText } of failureCases) {
+  for (const { imageName, stepText, message } of failureCases) {
     test(`failure ${imageName}`, async ({ page, browserName }) => {
       // eslint-disable-next-line playwright/no-skipped-test
       test.skip(browserName === 'webkit', 'safari. stop this')
@@ -280,10 +305,13 @@ test.describe('Image upload', () => {
 
       const step = page.getByTestId(`upload-step: ${stepText}`)
       await expect(step).toHaveAttribute('data-status', 'error', { timeout: 15000 })
-      await expectVisible(page, [
-        'text="Something went wrong. Please try again."',
-        'role=button[name="Back"]',
-      ])
+
+      await expect(page.getByText(message)).toBeVisible()
+      // confirm we don't show both the generic and a specific API message
+      if (message !== genericMessage) {
+        await expect(page.getByText(genericMessage)).toBeHidden()
+      }
+      await expect(page.getByRole('button', { name: 'Back' })).toBeVisible()
     })
   }
 })