Skip to content

Support canceling imports #3387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fakemonster wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Support canceling imports #3387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d773dd1
Support canceling imports
fakemonster Sep 15, 2026
e0dd917
Eagerly refresh disk state while canceling
fakemonster Sep 15, 2026
2bb465f
Use deterministic temp disk name for upload tests
fakemonster Sep 15, 2026
d0ce6d9
Add test for partial success in cancellation
fakemonster Sep 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/forms/image-upload.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ function getTmpDiskName(imageName: string) {
'import-start-500',
'import-stop-500',
'disk-finalize-500',
'cancel-upload',
])
if (specialNames.has(imageName)) return imageName
}
Expand Down
77 changes: 61 additions & 16 deletions app/pages/project/disks/DisksPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { useQuery } from '@tanstack/react-query'
import { createColumnHelper } from '@tanstack/react-table'
import { useCallback, useMemo } from 'react'
import { Outlet, type LoaderFunctionArgs } from 'react-router'
import { match } from 'ts-pattern'

import {
api,
Expand All @@ -28,6 +29,7 @@ import { DiskStateBadge, DiskTypeBadge, ReadOnlyBadge } from '~/components/State
import { makeCrumb } from '~/hooks/use-crumbs'
import { getProjectSelector, useProjectSelector } from '~/hooks/use-params'
import { useQuickActions } from '~/hooks/use-quick-actions'
import { confirmAction } from '~/stores/confirm-action'
import { confirmDelete } from '~/stores/confirm-delete'
import { addToast } from '~/stores/toast'
import { DiskSourceName } from '~/table/cells/DiskSourceCell'
Expand Down Expand Up @@ -113,6 +115,17 @@ export default function DisksPage() {
},
})

const { mutateAsync: finalize } = useApiMutation(api.diskFinalizeImport, {
onSuccess() {
queryClient.invalidateEndpoint('diskList')
},
})
const { mutateAsync: stopBulkWriteImport } = useApiMutation(api.diskBulkWriteImportStop, {
onSuccess() {
queryClient.invalidateEndpoint('diskList')
},
})

const makeActions = useCallback(
(disk: Disk): MenuAction[] => [
{
Expand All @@ -131,23 +144,55 @@ export default function DisksPage() {
},
disabled: snapshotDisabledReason(disk),
},
{
label: 'Delete',
onActivate: confirmDelete({
doDelete: () => deleteDisk({ path: { disk: disk.name }, query: { project } }),
label: disk.name,
resourceKind: 'disk',
}),
disabled:
!diskCan.delete(disk) &&
(disk.state.state === 'attached' ? (
'Disk must be detached before it can be deleted'
) : (
<>Only disks in state {fancifyStates(diskCan.delete.states)} can be deleted</>
)),
},
match(disk.state.state)
.with('import_ready', 'importing_from_bulk_writes', () => ({
label: 'Cancel import',
onActivate() {
confirmAction({
doAction: async () => {
if (disk.state.state === 'importing_from_bulk_writes') {
await stopBulkWriteImport({
path: { disk: disk.name },
query: { project },
})
}

await finalize({
path: { disk: disk.name },
query: { project },
body: {},
})

@david-crespo david-crespo Sep 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fable had an interesting point: if the disk has changed state, you might get an error trying to cancel import on an already-detached disk, when you could have just done a noop and said "tada!" It suggested fetching the disk again before doing anything to get the latest state. It's not bad, it's kinda cute. And I kind of like the match over the conditional skipping of the first step. Not sure about keeping the message the same whether we did anything or not. It's fine I guess?

const path = { disk: disk.name }
const query = { project }
// The row's state may be stale, e.g., an upload in another tab
// has moved on since the list loaded. Fetch the disk fresh so
// we make the right calls for its actual state.
const fresh = await queryClient.fetchQuery(q(api.diskView, { path, query }))
await match(fresh.state.state)
  .with('importing_from_bulk_writes', async () => {
    await stopBulkWriteImport({ path, query })
    await finalize({ path, query, body: {} })
  })
  .with('import_ready', () => finalize({ path, query, body: {} }))
  // already out of import mode, nothing to do but refresh the list
  .otherwise(() => queryClient.invalidateEndpoint('diskList'))

addToast(
  <>
    Import canceled for <HL>{disk.name}</HL>
  </>
)

@fakemonster fakemonster Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the disk has changed state, you might get an error

certainly this is the case with any action, right? I assume you can't stop a stopped instance, delete a deleted disk, etc.

beyond that, if we do refetch state, we should do the exact opposite: nothing, and say we did nothing. for instance, the state may have changed because the upload succeeded!

@david-crespo david-crespo Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that code, if it does something it's only because the new state is still importing. But yeah, that's a good point, though I think the idea is that some other resources that are liable to change do polling to keep the state relatively up to date. But those are more actively transitional, like instance starting or support bundle collecting. I could definitely live with doing nothing here instead of adding the latency of an extra fetch up front.


addToast(
<>
Import canceled for <HL>{disk.name}</HL>
</>
)
},
modalTitle: 'Cancel import',
modalContent: `Are you sure you want to cancel import for ${disk.name}?`,

@david-crespo david-crespo Sep 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also explain what's going to happen. Not sure about the level of detail, could be "It's going to end up detached" or "First it's going to stop the import and then it's going to finalize it." Second seems like too much info.

@fakemonster fakemonster Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Are you sure you want to cancel import for disk-1? This will detach your disk" seems confusing. Maybe we flip the script: the action here is generally referred to as "detaching", and the modal says "Are you sure you want to detach disk-1? This will cancel any ongoing import."

@fakemonster fakemonster Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that helps benefit the case we were discussing on the issue: understanding that what you're seeing could be a disk mid-healthy-upload

@david-crespo david-crespo Sep 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Detach" as a verb doesn't work here because the importing state is distinct from the attached state, which means attached to an instance. Here there's nothing it's being detached from. What I was trying to do is sort of telegraph the state it would end up in, which is kind of the generic Doing Nothing state, hence its terminal position in the beautiful state graph. Still, unless the user understands that that's what detached is (which we should not assume), telling them that might raise more questions than it answers. So maybe "cancel import" is about as good as it gets. The most literal version would be "get out of the importing state", but that's not really any better unless you're looking at the graph.

Image

errorTitle: 'Failed to cancel import',
actionType: 'danger',
})
},
}))
.otherwise(() => ({
label: 'Delete',
onActivate: confirmDelete({
doDelete: () => deleteDisk({ path: { disk: disk.name }, query: { project } }),
label: disk.name,
resourceKind: 'disk',
}),
disabled:
!diskCan.delete(disk) &&
(disk.state.state === 'attached' ? (
'Disk must be detached before it can be deleted'
) : (
<>Only disks in state {fancifyStates(diskCan.delete.states)} can be deleted</>
)),
})),

@david-crespo david-crespo Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the time we like to leave delete there but disabled with a nice message explaining why. I think that would be helpful in the importing case just as it is in the rest of the non-detached states.

],
[createSnapshot, deleteDisk, project]
[createSnapshot, deleteDisk, stopBulkWriteImport, finalize, project]
)

const columns = useColsWithActions(
Expand Down
43 changes: 43 additions & 0 deletions mock-api/disk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,49 @@ export const disks: Json<Disk>[] = [
disk_type: 'distributed',
read_only: false,
},
{
id: '7b898827-35a1-4459-a4e3-34db90640b74',
name: 'tmp-for-image-29884739',
description: 'stuck in import_ready after bailing on an image upload early',
project_id: project.id,
time_created: new Date().toISOString(),
time_modified: new Date().toISOString(),
state: { state: 'import_ready' },
device_path: '/import',
size: 8 * GiB,
block_size: 2048,
disk_type: 'distributed',
read_only: false,
},
{
id: 'f874c0b9-72ad-4eac-8e55-e6090e10366a',
name: 'tmp-for-image-59986861',
description: 'stuck in bulk-write after bailing on an image upload early',
project_id: project.id,
time_created: new Date().toISOString(),
time_modified: new Date().toISOString(),
state: { state: 'importing_from_bulk_writes' },
device_path: '/import',
size: 8 * GiB,
block_size: 2048,
disk_type: 'distributed',
read_only: false,
},
{
id: '0f60c28e-ead0-48f0-aab9-e74b917dc8e4',
name: 'disk-finalize-fail',
description:
"stuck in bulk-write after bailing on an image upload early, but can't be finalized",
project_id: project.id,
time_created: new Date().toISOString(),
time_modified: new Date().toISOString(),
state: { state: 'importing_from_bulk_writes' },
device_path: '/import',
size: 8 * GiB,
block_size: 2048,
disk_type: 'distributed',
read_only: false,
},
{
id: '3f23c80f-c523-4d86-8292-2ca3f807bb12',
name: 'disk-snapshot-error',
Expand Down
4 changes: 3 additions & 1 deletion mock-api/msw/handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,9 @@ export const handlers = makeHandlers({
diskFinalizeImport: ({ path, query, body }) => {
const disk = lookup.disk({ ...path, ...query })

if (disk.name === 'disk-finalize-500') throw internalError('disk finalize failed')
if (disk.name === 'disk-finalize-500' || disk.name === 'disk-finalize-fail') {
throw internalError('disk finalize failed')
}

if (disk.state.state !== 'import_ready') {
throw `Cannot finalize disk in state ${disk.state.state}. Must be import_ready.`
Expand Down
65 changes: 64 additions & 1 deletion test/e2e/disks.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/
import {
clickRowAction,
clickRowActions,
expect,
expectNoToast,
expectRowVisible,
Expand Down Expand Up @@ -93,7 +94,7 @@ test('List disks and snapshot', async ({ page }) => {
await page.goto('/projects/mock-project/disks')

const table = page.getByRole('table')
await expect(table.getByRole('row')).toHaveCount(16) // 15 + header
await expect(table.getByRole('row')).toHaveCount(19) // 18 + header

// check one attached and one not attached
await expectRowVisible(table, {
Expand Down Expand Up @@ -166,6 +167,68 @@ test('Read-only disk snapshot disabled', async ({ page }) => {
)
})

test('Cancel import from import_ready', async ({ page }) => {
const diskImportReadyName = 'tmp-for-image-29884739'
await page.goto('/projects/mock-project/disks')
const table = page.getByRole('table')
await expectRowVisible(table, { name: diskImportReadyName, state: 'import ready' })

await clickRowActions(page, diskImportReadyName)
await expect(page.getByRole('menuitem', { name: 'Delete' })).toBeHidden()
await page.getByRole('menuitem', { name: 'Cancel import' }).click()

const modal = page.getByRole('dialog', { name: 'Cancel import' })
await expect(modal).toBeVisible()
await modal.getByRole('button', { name: 'Confirm' }).click()

await expectToast(page, `Import canceled for ${diskImportReadyName}`)
await expectRowVisible(table, { name: diskImportReadyName, state: 'detached' })
await clickRowActions(page, diskImportReadyName)
await expect(page.getByRole('menuitem', { name: 'Delete' })).toBeVisible()
})

test('Cancel import from importing_from_bulk_writes', async ({ page }) => {
const diskImportingName = 'tmp-for-image-59986861'
await page.goto('/projects/mock-project/disks')
const table = page.getByRole('table')
await expectRowVisible(table, {
name: diskImportingName,
state: 'importing from bulk writes',
})

await clickRowActions(page, diskImportingName)
await expect(page.getByRole('menuitem', { name: 'Delete' })).toBeHidden()
await page.getByRole('menuitem', { name: 'Cancel import' }).click()

const modal = page.getByRole('dialog', { name: 'Cancel import' })
await expect(modal).toBeVisible()
await modal.getByRole('button', { name: 'Confirm' }).click()

await expectToast(page, `Import canceled for ${diskImportingName}`)
await expectRowVisible(table, { name: diskImportingName, state: 'detached' })
await clickRowActions(page, diskImportingName)
await expect(page.getByRole('menuitem', { name: 'Delete' })).toBeVisible()
})

test('Cancel import from importing_from_bulk_writes shows error and refreshes state when finalize fails', async ({
page,
}) => {
const diskName = 'disk-finalize-fail'
await page.goto('/projects/mock-project/disks')
const table = page.getByRole('table')
await expectRowVisible(table, { name: diskName, state: 'importing from bulk writes' })

await clickRowActions(page, diskName)
await page.getByRole('menuitem', { name: 'Cancel import' }).click()
await page
.getByRole('dialog', { name: 'Cancel import' })
.getByRole('button', { name: 'Confirm' })
.click()

await expectToast(page, 'Failed to cancel import')
await expectRowVisible(table, { name: diskName, state: 'import ready' })
})

test.describe('Disk create', () => {
test.beforeEach(async ({ page }) => {
await page.goto('/projects/mock-project/disks-new')
Expand Down
6 changes: 4 additions & 2 deletions test/e2e/image-upload.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ test.describe('Image upload', () => {

for (const state of cancelStates) {
test(`cancel in state '${state}'`, async ({ page }) => {
await fillForm(page, 'new-image')
await fillForm(page, 'cancel-upload')

await page.getByRole('button', { name: 'Upload image' }).click()

Expand All @@ -219,7 +219,9 @@ test.describe('Image upload', () => {
await page.getByRole('button', { name: 'Cancel' }).click()
await page.getByRole('link', { name: 'Disks' }).click()
await expect(page.getByRole('cell', { name: 'disk-1', exact: true })).toBeVisible()
await expect(page.getByRole('cell', { name: 'tmp' })).toBeHidden()
await expect(
page.getByRole('cell', { name: 'cancel-upload', exact: true })
).toBeHidden()
})
}

Expand Down
Loading