Support canceling imports - #3387
fakemonster wants to merge 4 commits into
Conversation
This constitutes two closely-related workflows: 1. Finalizing a disk from `import_ready` to `detached`. You can get yourself stuck in `import_ready` by starting and immediately abandoning an image upload (which creates a temp disk). 2. Canceling a bulk write and _then_ finalizing a disk, moving from `importing_from_bulk_writes` to `detached`. Same activity, but you get to that stuck state by abandoning an image upload after the actual upload starts. While we won't pretend the states are same when viewing them, "doing something about it" is basically the same thing in both cases, even if the API actions are slightly different.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Would be cool to have an e2e test exercise the try/catch that wraps the action function by failing one of the requests. |
Kind of a middle ground between "show two toasts for this action" and "pretend we aren't making two api calls".
sure, i'll have one with finalize failing so we can also assert that the state still updates to the middle point |
| ) | ||
| }, | ||
| modalTitle: 'Cancel import', | ||
| modalContent: `Are you sure you want to cancel import for ${disk.name}?`, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
"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."
There was a problem hiding this comment.
that helps benefit the case we were discussing on the issue: understanding that what you're seeing could be a disk mid-healthy-upload
There was a problem hiding this comment.
"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.
| ) : ( | ||
| <>Only disks in state {fancifyStates(diskCan.delete.states)} can be deleted</> | ||
| )), | ||
| })), |
There was a problem hiding this comment.
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.
| path: { disk: disk.name }, | ||
| query: { project }, | ||
| body: {}, | ||
| }) |
There was a problem hiding this comment.
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>
</>
)There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
2 participants
This constitutes two closely-related workflows:
import_readytodetached. You can get yourself stuck inimport_readyby starting and immediately abandoning an image upload (which creates a temp disk).importing_from_bulk_writestodetached. Same activity, but you get to that stuck state by abandoning an image upload after the actual upload starts.While we won't pretend the states are same when viewing them, "doing something about it" is basically the same thing in both cases, even if the API actions are slightly different.
Resolves #1568