supabase / storage

S3 compatible object storage service that stores metadata in Postgres
https://supabase.com/docs/guides/storage
Apache License 2.0
764 stars 107 forks source link

5-10% of Uppy/TUS uploads fail #419

Closed lmyslinski closed 8 months ago

lmyslinski commented 8 months ago

I'm trying to use Supabase Storage with uploads via Uppy. Roughly 5-10% of my uploads fail, even though Uppy reports that the upload was completed. At this point, I'm actively looking for a more stable alternative. I've spent a couple of hours working around various issues, I'll try to describe everything I've encountered. I've used https://github.com/supabase/supabase/tree/master/examples/storage/resumable-upload-uppy as a base.

So when I upload a file via Uppy with TUS, Uppy reports that the upload was successful. Yet, when I subsequently try to access the upload file on my backed, the file is not found (resulting in the 500 seen on the screenshot).

image

API error from the screenshot:

Moving the file uploads/3637052f-8d83-4c to verified/9eaa91fd-da80-4dbc-96b0-83effe600e63
StorageApiError: Object not found
    at eval (webpack-internal:///(rsc)/./node_modules/.pnpm/@supabase+storage-js@2.5.5/node_modules/@supabase/storage-js/dist/module/lib/fetch.js:44:24)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  __isStorageError: true,
  status: 400
}

API code:

export const moveTemplateToVerified = async (
  tmpFileName: string,
  templateId: string
) => {
  console.log(
    `Moving the file uploads/${tmpFileName} to verified/${templateId}`
  );

  const { data, error } = await supabase.storage
    .from("templates")
    .move(`uploads/${tmpFileName}`, `verified/${templateId}.docx`);

  if (error) {
    throw error;
  }
};

This happens roughly 5-10% of the time. This is a big no-go for me and I'm currently looking for a more stable alternative. I believe this is likely due to TUS implementation issues, I'd appreciate a recommendation for a more stable API. At this point I'm wondering whether using pre-signed URL's for uploads + XHR uploads from the server will be more stable. There's also more stuff that's problematic:

Here's the upload component:

"use client";

import Uppy, { PluginOptions, UploadResult, UppyFile } from "@uppy/core";
import { Dashboard } from "@uppy/react";
import Tus from "@uppy/tus";
import { v4 as uuidv4 } from "uuid";

import "@uppy/core/dist/style.min.css";
import "@uppy/dashboard/dist/style.min.css";
import { useState } from "react";

interface TemplateUploadProps {
  setTemplateName: (name: string) => void;
  setTmpUploadFileName: (name: string) => void;
}

const SUPABASE_ANON_KEY =
  "..."; // your project's anon key
const SUPABASE_PROJECT_ID = "..."; // your project ref
const STORAGE_BUCKET = "..."; // your storage bucket name

const supabaseStorageURL = `https://${SUPABASE_PROJECT_ID}.supabase.co/storage/v1/upload/resumable`;

const tusConfig = {
  endpoint: supabaseStorageURL,
  headers: {
    authorization: `Bearer ${SUPABASE_ANON_KEY}`,
    apikey: SUPABASE_ANON_KEY,
  },
  uploadDataDuringCreation: true,
  chunkSize: 6 * 1024 * 1024,
  allowedMetaFields: [
    "bucketName",
    "objectName",
    "contentType",
    "cacheControl",
  ],
  onerror: (error: any) => console.log("tus error", error),
};

export default function TemplateUpload({
  setTmpUploadFileName,
  setTemplateName,
}: TemplateUploadProps) {
  const [isUploadComplete, setIsUploadComplete] = useState(false);

  const generateFileName = () => {
    const uuid = uuidv4();
    const name = uuid.substring(0, 16);
    console.log("generated name", name);
    return name;
  };

  const renameFile = (currentFile: UppyFile, files: any) => {
    const modifiedFile = {
      ...currentFile,
      name: generateFileName(),
    };
    setTemplateName(currentFile.name);
    console.log("Uploading file: ", modifiedFile.name);
    return modifiedFile;
  };

  const handleUploadResult = (result: UploadResult) => {
    console.log("Upload result:", result);
    if (result.failed.length > 0) {
      console.log("Upload failed", result.failed);
      return;
    } else if (result.successful.length > 0) {
      setIsUploadComplete(true);
      setTmpUploadFileName(`${result.successful[0].name}`);
    }
  };

  // IMPORTANT: passing an initializer function to prevent Uppy from being reinstantiated on every render.
  const [uppy] = useState(() =>
    new Uppy({
      onBeforeFileAdded: renameFile,
      allowMultipleUploadBatches: false,
      restrictions: {
        maxFileSize: 10000000, // 10mb
        maxNumberOfFiles: 1,
        minNumberOfFiles: 1,
        // allowedFileTypes: ["application/pdf"],
      },
      autoProceed: true,
    }).use(Tus, tusConfig)
  );

  uppy.on("file-added", (file) => {
    const supabaseMetadata = {
      bucketName: STORAGE_BUCKET,
      objectName: `uploads/${file.name}`,
      contentType: file.type,
    };

    file.meta = {
      ...file.meta,
      ...supabaseMetadata,
    };

    console.log("file added", file);
  });

  // uppy.on("upload-success", (file, response) => {
  //   console.log("Upload success", file, response);
  // });

  uppy.on("complete", handleUploadResult);

  return (
    <>
      {isUploadComplete && <div>Upload Complete!</div>}
      {!isUploadComplete && <Dashboard width={600} height={400} uppy={uppy} />}
    </>
  );
}
fenos commented 8 months ago

Hello @lmyslinski thanks a lot for your feedback! Let's go step by step. Most of the issues are not real issues and how the protocol is supposed to work.

It's impossible to reupload files - I have to rename the files during the upload, as otherwise we get a 409, which I didn't manage to get around with custom policies

This is expected, since uploading a file on the same path (essentially overwriting a file) you are required to provide the x-upsert=true header, if you don't pass this header storage will not allow you to upload to a path where a file already exists.

However, we do strongly recommend using the pattern of creating a unique name for each of the files like you are doing, since overwriting files on the same path will not bypass the browser cache and the CDN cache takes a little time to propagate the changes

See documentation: https://supabase.com/docs/guides/storage/uploads/resumable-uploads#overwriting-files

if the file name/metadata doesn't match between supabase and uppy, the upload just fails with a 404 - pretty much impossible to debug, I've fixed this with trial & error

Could you explain this further, not sure i understood this correctly

every successful TUS upload involves 404 calls - this should not be a part of a successful data flow

This is how the protocol works, when you start the upload of a file, tus creates a fingerprint and stores it in the local-storage, when you stop the upload and resume it later, TUS will re-use the same upload URL. it first sends a HEAD request to receive back the upload offset so that it can resume where it left-off. In case the upload is successful by default TUS doesn't remove that fingerprint from the local-storage see removeFingerprintOnSuccess, if you delete the file later on, it will try to call the same upload URL but it is now deleted so you'll receive a 404 and it will create a new upload url automatically.

So when I upload a file via Uppy with TUS, Uppy reports that the upload was successful. Yet, when I subsequently try to access the upload file on my backed, the file is not found

This issue is also caused by the flag removeFingerprintOnSuccess if not set to true it uses the default value false.

When the upload succeeds the browser still keeps the fingerprint of the file in the local-storage. Due to a limitation of tus-node-server it is currently advised to set this value to true , this should solve all the stability problems you are currently having with Resumable upload, since once the browser completes the upload will not be holding that reference in the local-storage for future lookups.

This happens roughly 5-10% of the time. This is a big no-go for me and I'm currently looking for a more stable alternative. I believe this is likely due to TUS implementation issues, I'd appreciate a recommendation for a more stable API

The protocol implementation is a Beta feature in our current platform so there are currently small perks like the above.

However, we have contributed back to the TUS protocol a lot in the past months and we have improved stability dramatically, as a sneak-peak we will announce TUS to be out of beta very soon, entering a stable release.

We are also working on having TUS uploads embedded in the Supabase SDK so that we can set default values more consistently, (this will come a bit later the stable release)

for example:

await supbase.storage.from('bucket').resumableUpload('path', file, {
  onProgress: (uploaded, remaining) => {},
}) 

the UI for storage is super slow Could you please send a support ticket i would be happy to look into this personally for you

Again, thanks a lot for the feedback. I Hope with adding removefingerprintonsuccess: true and the upcoming stable release of TUS you'll have a great experience with Storage

Let me know if you have any more questions

lmyslinski commented 8 months ago

@fenos Thanks for the detailed response. I understand it's in beta - for now I've switched to XHR which works pretty well. I'll soon need to add bulk upload support so I'll give it another shot with removeFingerprintOnSuccess

fenos commented 8 months ago

Awesome! Closing for now, i'll update this issue when we release TUS stable release.

However, feel free to also update me here for anything else. In case we'll just re-open it. Thanks @lmyslinski

fenos commented 7 months ago

@lmyslinski resumable upload is now considered stable! 🎉 All the issues above mentioned are now resolved

AkramEld commented 7 months ago

@lmyslinski resumable upload is now considered stable! 🎉 All the issues above mentioned are now resolved

Hi im using "@uppy/core": "^3.9.2",. Im experiencing what lmyslinski explained. uppy is saying the image upload succeeded but the actual image is not being uploaded to supabase. I've noticed this is only happening to some images

1-Felix commented 5 months ago

Hi im using "@uppy/core": "^3.9.2",. Im experiencing what lmyslinski explained. uppy is saying the image upload succeeded but the actual image is not being uploaded to supabase. I've noticed this is only happening to some images

I run in into this issue too, where the upload seems to succeed but nothing gets uploaded to Supabase. Clearing the cache/localstorage seems to fix the issue, for me. But not sure what's the cause exactly.

Wizzel1 commented 4 months ago

@lmyslinski resumable upload is now considered stable! 🎉 All the issues above mentioned are now resolved

Hi im using "@uppy/core": "^3.9.2",. Im experiencing what lmyslinski explained. uppy is saying the image upload succeeded but the actual image is not being uploaded to supabase. I've noticed this is only happening to some images

Also happens to me with uppy ^3.11.0

1-Felix commented 4 months ago

Hi im using "@uppy/core": "^3.9.2",. Im experiencing what lmyslinski explained. uppy is saying the image upload succeeded but the actual image is not being uploaded to supabase. I've noticed this is only happening to some images

I run in into this issue too, where the upload seems to succeed but nothing gets uploaded to Supabase. Clearing the cache/localstorage seems to fix the issue, for me. But not sure what's the cause exactly.

I think I found my issue. I'll document it here in case anyone comes across this.

Totally not an expert, so take it with a bit of salt.

I'm using the TUS client in combination with Supabase and React. It seems that Uppy stores some data about the file to upload in localStorage. In my case, the problem was that uploading the same file twice in separate Uppy instances would not create a new localStorage entry. This, in turn, meant that Uppy assumed this File was already uploaded and just returned successfully. This interfered with my application logic, where I actually want to allow uploading the same file twice and treat them as completely separate entities.

The fix, for me, was to overwrite the file.id for each file added to Uppy to give it a unique ID, which meant that a new localStorage entry would be created every time.

It looks something like this:

new Uppy({
  autoProceed: true,
  id: uppyId,
  debug: false,
  onBeforeFileAdded(currentFile) {
    const newItemId = crypto.randomUUID();
    const modifiedFile = {
      ...currentFile,
      id: `${currentFile.id}-${newItemId}`, // <--- the important part
    };

    return modifiedFile;
  },
})

The entierty of my Upload-Area code can be found below, in case anyone is curious. Be warned, it's a bit wild (and probably wrong)

The entire example

```tsx type CustomMeta = { itemId?: UUID; bucketName?: string; objectName?: string; contentType?: string; }; export function UploadArea({ dispatch, tierlistData, pathname, }: { dispatch: (action: tierListActions) => void; tierlistData: OptimisticTierlist; pathname: string; }) { const supabase = createClient(); // https://github.com/transloadit/uppy/issues/3185 const tierlistDataRef = useRef(tierlistData); // Update ref on every render useEffect(() => { tierlistDataRef.current = tierlistData; }); const [uppy] = useState(() => new Uppy({ autoProceed: true, id: tierlistData.id, debug: false, onBeforeFileAdded(currentFile) { const newItemId = crypto.randomUUID(); const modifiedFile = { ...currentFile, id: `${currentFile.id}-${newItemId}`, // Unique ID is important, so that same file can be used in different tierlists }; return modifiedFile; }, }) .use(Compressor, { width: 80, convertSize: MAX_FILE_SIZE, // If image is larger than 10MB, convert to JPEG }) .use(ThumbnailGenerator, { thumbnailWidth: 80, thumbnailType: "image/png", waitForThumbnailsBeforeUpload: true, // This is important so that we have an up to date React State when uploading }) .use(Tus, { endpoint: process.env.NEXT_PUBLIC_SUPABASE_STORAGE_UPLOAD_URL, headers: { Authorization: `Bearer ${process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY}`, // "x-upsert": "true", // optionally set upsert to true to overwrite existing files (not recommended because CDN Cache takes longer to update) }, uploadDataDuringCreation: true, onAfterResponse: function (req, res) { const url = req.getURL(); const value = res.getStatus(); if (res.getStatus() === 409) { console.info("⛷️ File already exists, skipping image upload"); } else if (res.getStatus() >= 400) { console.info(`🚨 Request for ${url} responded with ${value}`); } }, // removeFingerprintOnSuccess: true, // Important to allow re-uploading the same file https://github.com/tus/tus-js-client/blob/main/docs/api.md#removefingerprintonsuccess chunkSize: 6 * 1024 * 1024, // NOTE: it must be set to 6MB (for now) do not change it https://supabase.com/docs/guides/storage/uploads/resumable-uploads allowedMetaFields: ["bucketName", "objectName", "contentType", "cacheControl", "name"], }) .on("file-added", (file) => { const folder = tierlistData.id; const supabaseMetadata = { bucketName: ITEMS_STORAGE_BUCKET, objectName: folder ? `${folder}/${file.name}` : file.name, contentType: file.type, }; file.meta = { ...file.meta, ...supabaseMetadata, }; console.debug("1️⃣ file added", file); }) .on("thumbnail:generated", (file, preview) => { console.debug("2️⃣ thumbnail generated", file, preview); const unsortedTier = tierlistDataRef.current.tiers.find( ({ unsorted_tier }) => unsorted_tier ); if (file.meta.itemId && unsortedTier?.id) { dispatch({ type: "add_item", payload: { item: { id: file.meta.itemId, name: file.meta.itemId, previewSrc: preview, src: `${ITEMS_STORAGE_BUCKET}/${file.meta.objectName}`, tier_id: unsortedTier.id, }, }, }); } else { toast.error("Something went wrong, please try again."); uppy.cancelAll(); } return file; }) .on("upload-error", (file, error) => { console.error("👹 Upload error", file, error); }) .on("upload-success", async (file, response) => { const unsortedTier = tierlistDataRef.current.tiers.find( ({ unsorted_tier }) => unsorted_tier ); console.debug("3️⃣ upload success", file, response); const { id, name, position } = // This items array is only guaranteed to be up to date, if waitForThumbnailsBeforeUpload = true unsortedTier?.items.find((item) => item.id === file?.meta.itemId) ?? {}; try { if (!id || !name || position === undefined) { throw new Error("👹 Item not found in state"); } if (file?.name && unsortedTier?.id) { await upsertTierListAction(tierlistDataRef.current, pathname); console.debug("4️⃣ item updated in DB"); } } catch (error) { console.error("👹 Error saving item", error); if (file) { toast.error(`Failed to save ${file.name}, please try again.`, { duration: 5000 }); dispatch({ type: "remove_item", payload: { id: file.meta.itemId } }); if (file.meta.objectName) { await supabase.storage.from(ITEMS_STORAGE_BUCKET).remove([file.meta.objectName]); } } } }) .on("complete", ({ failed, successful }) => { console.log("🎉 successful:", successful, "failed:", failed); if (failed.length > 0) { toast.error( `Failed to upload file(s) ${failed.map((f) => f.name).join(", ")}, please try again.`, { duration: Math.min(3500 + failed.length * 500, 5000) } ); failed.forEach((f) => { dispatch({ type: "remove_item", payload: { id: f.meta.itemId } }); }); } else if (successful.length > 0) { toast.success("Saved image(s) successfully"); } }) ); return (

); } ```

rafal-r commented 2 months ago

Same issue here for Supabase uploads using Tus. Uppy reports succesful upload for some images and videos files but they do not end up in the buckets.

My packages versions:

    "@uppy/audio": "^2.0.0",
    "@uppy/compressor": "^2.0.0",
    "@uppy/core": "^4.0.1",
    "@uppy/dashboard": "^4.0.1",
    "@uppy/drag-drop": "^4.0.1",
    "@uppy/drop-target": "^3.0.1",
    "@uppy/file-input": "^4.0.0",
    "@uppy/golden-retriever": "^4.0.0",
    "@uppy/image-editor": "^3.0.0",
    "@uppy/progress-bar": "^4.0.0",
    "@uppy/react": "^4.0.1",
    "@uppy/screen-capture": "^4.0.0",
    "@uppy/tus": "^4.0.0",
    "@uppy/webcam": "^4.0.0",

And I'm running locally Supabase 1.187.3.

Thankfully the fix mentioned by @1-Felix helped 🙌! Maybe it's worth reopening the ticket and checking the working solution @fenos.