feat(s3): bring generic S3 to parity with R2 (cleanup + CORS guidance)

Two gaps where generic S3 trailed Cloudflare R2:

1. Object cleanup on delete/edit didn't work for generic S3. deleteVideoStorage
   built its client via makeR2Client, which hard-forces path-style and ignores
   the connection's forcePathStyle -- so vhost-style S3 deletes were misaddressed
   and cleanup silently failed (S3 users' deleted/edited files orphaned). It is
   now provider-aware (mirrors uploadFile): R2 -> makeR2Client (path-style),
   generic S3 -> makeS3Client honoring forcePathStyle. makeS3Client is now
   injectable for testing.

2. CORS guidance on upload failure was R2-only. Extracted isLikelyCorsError +
   a generic-S3 guidance builder into js/utils/uploadErrorHints.js (re-exported
   from r2Service for back-compat); s3UploadService now appends actionable CORS
   guidance on an opaque "Failed to fetch" too.

Generic S3 is now at functional parity with R2 for upload -> publish -> cleanup.

Tests: r2-delete-video-storage (adds generic-S3 forcePathStyle routing),
s3-upload-cors-guidance (new). Full suite green.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

js/services/r2Service.js

@@ -45,6 +45,7 @@ import {
   collectVideoStorageKeys,
 } from "../utils/storagePointer.js";
 import { safeEncodeNpub } from "../utils/nostrHelpers.js";
+import { isLikelyCorsError } from "../utils/uploadErrorHints.js";
 import {
   getVideoNoteErrorMessage,
   normalizeVideoNotePayload,
@@ -103,24 +104,9 @@ function buildCorsGuidance({ accountId } = {}) {
   ].join(" ");
 }
 
-/**
- * A browser CORS preflight/rejection surfaces as an opaque network error
- * ("Failed to fetch" / "NetworkError" / Safari's "Load failed") with no HTTP
- * status — indistinguishable from a real network drop. We treat these as
- * likely-CORS so the upload path can attach actionable guidance instead of a
- * bare "Failed to fetch".
- */
-export function isLikelyCorsError(err) {
-  const message = (
-    (err && (err.message || err.toString && err.toString())) ||
-    ""
-  ).toLowerCase();
-  return (
-    message.includes("failed to fetch") ||
-    message.includes("networkerror") ||
-    message.includes("load failed")
-  );
-}
+// Shared CORS-error classifier (also used by s3UploadService). Re-exported here
+// for backward compatibility with existing imports/tests.
+export { isLikelyCorsError };
 
 function createDefaultSettings() {
   return {
@@ -163,6 +149,7 @@ function safeDecodeNpub(npub) {
 export class R2Service {
   constructor({
     makeR2Client: makeR2ClientOverride,
+    makeS3Client: makeS3ClientOverride,
     multipartUpload: multipartUploadOverride,
     ensureBucketExists: ensureBucketExistsOverride,
     ensureBucketCors: ensureBucketCorsOverride,
@@ -171,6 +158,7 @@ export class R2Service {
     this.listeners = new Map();
     this.cloudflareSettings = null;
     this.makeR2Client = makeR2ClientOverride || makeR2Client;
+    this.makeS3Client = makeS3ClientOverride || makeS3Client;
     this.multipartUpload = multipartUploadOverride || multipartUpload;
     this.ensureBucketExists = ensureBucketExistsOverride || ensureBucketExists;
     this.ensureBucketCors = ensureBucketCorsOverride || ensureBucketCors;
@@ -260,13 +248,29 @@ export class R2Service {
     let s3;
     try {
       await ensureS3SdkLoaded();
-      s3 = this.makeR2Client({
-        accountId: settings.accountId,
-        endpoint: settings.endpoint,
-        accessKeyId,
-        secretAccessKey,
-        region: settings.region,
-      });
+      // Provider-aware client: Cloudflare R2 is always path-style; a generic S3
+      // bucket may be virtual-hosted-style, so honor the connection's
+      // forcePathStyle (mirrors uploadFile). Using makeR2Client unconditionally
+      // here forced path-style and broke cleanup for vhost-style S3 buckets.
+      const useR2 =
+        settings.provider === "cloudflare_r2" || Boolean(settings.accountId);
+      if (useR2) {
+        s3 = this.makeR2Client({
+          accountId: settings.accountId,
+          endpoint: settings.endpoint,
+          accessKeyId,
+          secretAccessKey,
+          region: settings.region,
+        });
+      } else {
+        s3 = this.makeS3Client({
+          endpoint: settings.endpoint,
+          region: settings.region,
+          accessKeyId,
+          secretAccessKey,
+          forcePathStyle: Boolean(settings.forcePathStyle),
+        });
+      }
     } catch (err) {
       devLogger.warn(
         "[R2Service] deleteVideoStorage: failed to build storage client:",

js/services/s3UploadService.js

@@ -18,6 +18,10 @@ import {
   VIDEO_NOTE_ERROR_CODES,
 } from "./videoNotePayload.js";
 import { calculateTorrentInfoHash } from "../utils/torrentHash.js";
+import {
+  isLikelyCorsError,
+  buildGenericCorsGuidance,
+} from "../utils/uploadErrorHints.js";
 
 const STATUS_VARIANTS = new Set(["info", "success", "error", "warning"]);
 const INFO_HASH_PATTERN = /^[a-f0-9]{40}$/;
@@ -510,10 +514,15 @@ export class S3UploadService {
       return Boolean(published);
     } catch (err) {
       this.deps.userLogger.error("S3 upload failed:", err);
-      this.setUploadStatus(
-        err?.message ? `Upload failed: ${err.message}` : "Upload failed.",
-        "error"
-      );
+      let message = err?.message
+        ? `Upload failed: ${err.message}`
+        : "Upload failed.";
+      // A CORS rejection during the browser PUT/POST is an opaque "Failed to
+      // fetch" — attach actionable guidance (parity with the R2 path).
+      if (isLikelyCorsError(err)) {
+        message += ` ${buildGenericCorsGuidance({ endpoint: normalized?.endpoint })}`;
+      }
+      this.setUploadStatus(message, "error");
       return false;
     } finally {
       this.setUploading(false);

js/utils/uploadErrorHints.js

@@ -0,0 +1,42 @@
+// Shared helpers for turning opaque upload failures into actionable guidance.
+// Used by both upload paths (Cloudflare R2 via r2Service, generic S3 via
+// s3UploadService) so the behavior stays in sync.
+
+/**
+ * A browser CORS preflight/rejection surfaces as an opaque network error
+ * ("Failed to fetch" / "NetworkError" / Safari's "Load failed") with no HTTP
+ * status — indistinguishable from a real network drop. We treat these as
+ * likely-CORS so the upload path can attach actionable guidance instead of a
+ * bare "Failed to fetch".
+ */
+export function isLikelyCorsError(err) {
+  const message = (
+    (err && (err.message || (err.toString && err.toString()))) ||
+    ""
+  ).toLowerCase();
+  return (
+    message.includes("failed to fetch") ||
+    message.includes("networkerror") ||
+    message.includes("load failed")
+  );
+}
+
+/**
+ * Provider-neutral CORS guidance for a generic S3 bucket. (Cloudflare R2 has a
+ * more specific variant in r2Service that names the R2 S3 API endpoint.)
+ */
+export function buildGenericCorsGuidance({ endpoint } = {}) {
+  const origin =
+    typeof window !== "undefined" &&
+    window.location &&
+    window.location.origin &&
+    window.location.origin !== "null"
+      ? window.location.origin
+      : "<your-app-origin>";
+  const target = endpoint ? endpoint : "your bucket's S3 API endpoint";
+  return [
+    "This is likely a CORS issue.",
+    `Configure CORS on ${target} to allow origin ${origin},`,
+    "with AllowedMethods: GET, HEAD, PUT, POST, DELETE, OPTIONS and AllowedHeaders: *.",
+  ].join(" ");
+}

tests/services/r2-delete-video-storage.test.mjs

@@ -18,6 +18,8 @@ const { R2Service } = await import("../../js/services/r2Service.js");
 
 const BASE = "https://pub.bitvid.network";
 const CREDS = {
+  provider: "cloudflare_r2",
+  accountId: "acct",
   accessKeyId: "AK",
   secretAccessKey: "SK",
   bucket: "my-bucket",
@@ -28,8 +30,16 @@ const CREDS = {
 
 function buildService(creds, deleteImpl) {
   const deleted = [];
+  const clients = []; // records which client builder was used (r2 vs s3) + args
   const svc = new R2Service({
-    makeR2Client: () => ({ __mock: true }),
+    makeR2Client: (args) => {
+      clients.push({ kind: "r2", args });
+      return { __mock: "r2" };
+    },
+    makeS3Client: (args) => {
+      clients.push({ kind: "s3", args });
+      return { __mock: "s3" };
+    },
     deleteObject:
       deleteImpl ||
       (async ({ key }) => {
@@ -37,11 +47,11 @@ function buildService(creds, deleteImpl) {
       }),
   });
   svc.resolveConnection = async () => creds;
-  return { svc, deleted };
+  return { svc, deleted, clients };
 }
 
-test("deletes the video, its .torrent, and the thumbnail for an owned video", async () => {
-  const { svc, deleted } = buildService(CREDS);
+test("deletes the video, its .torrent, and the thumbnail for an owned video (R2)", async () => {
+  const { svc, deleted, clients } = buildService(CREDS);
   const res = await svc.deleteVideoStorage({
     npub: "npub1owner",
     videos: [
@@ -54,6 +64,33 @@ test("deletes the video, its .torrent, and the thumbnail for an owned video", as
     new Set(["u/np/h/clip.mp4", "u/np/h/clip.torrent", "u/np/h/clip.thumb.jpg"]),
   );
   assert.equal(res.deleted.length, 3);
+  // R2 connection => R2 (path-style) client.
+  assert.equal(clients[0]?.kind, "r2");
+});
+
+test("uses a forcePathStyle-aware S3 client for a generic-S3 connection", async () => {
+  // No accountId, non-R2 provider, virtual-hosted-style bucket.
+  const s3Creds = {
+    provider: "s3",
+    accessKeyId: "AK",
+    secretAccessKey: "SK",
+    bucket: "my-bucket",
+    publicBaseUrl: BASE,
+    endpoint: "https://s3.us-east-1.amazonaws.com",
+    region: "us-east-1",
+    forcePathStyle: false,
+  };
+  const { svc, deleted, clients } = buildService(s3Creds);
+  const res = await svc.deleteVideoStorage({
+    npub: "npub1owner",
+    videos: [{ url: `${BASE}/u/np/h/clip.mp4` }],
+  });
+  assert.equal(res.skipped, false);
+  assert.ok(deleted.includes("u/np/h/clip.mp4"), "deletes the S3 object");
+  // Generic S3 => S3 client built with the connection's forcePathStyle (not forced).
+  assert.equal(clients[0]?.kind, "s3");
+  assert.equal(clients[0]?.args.forcePathStyle, false);
+  assert.equal(clients[0]?.args.endpoint, "https://s3.us-east-1.amazonaws.com");
 });
 
 test("skips entirely when storage is locked (no credentials available)", async () => {

tests/services/s3-upload-cors-guidance.test.mjs

@@ -0,0 +1,77 @@
+// Scenario (SCN-s3-upload-cors-guidance):
+//   Parity with the R2 path — when a generic-S3 upload fails with an opaque
+//   browser CORS rejection ("Failed to fetch"), the user-facing status must
+//   include actionable CORS guidance, not a bare "Upload failed: Failed to
+//   fetch". Ordinary errors must NOT get the CORS hint.
+
+import "../test-helpers/setup-localstorage.mjs";
+import test from "node:test";
+import assert from "node:assert/strict";
+
+import { S3UploadService } from "../../js/services/s3UploadService.js";
+
+function makeDeps(uploadImpl) {
+  return {
+    ensureS3SdkLoaded: async () => {},
+    makeS3Client: () => ({}),
+    multipartUpload: uploadImpl,
+    buildR2Key: () => "u/np/ns/video.mp4",
+    computeStorageContentHash: async () => "sha256deadbeef",
+    buildS3ObjectUrl: () => "https://mock.example/file",
+    getCorsOrigins: () => ["*"],
+    prepareS3Connection: async (o) => ({ ...o }),
+    validateS3Connection: (s) => ({
+      ...s,
+      endpoint: "https://s3.us-east-1.amazonaws.com",
+      region: "us-east-1",
+      accessKeyId: "k",
+      secretAccessKey: "s",
+      bucket: "b",
+      forcePathStyle: false,
+      publicBaseUrl: "https://mock.example",
+    }),
+    userLogger: { warn() {}, error() {} },
+    buildStoragePointerValue: () => "ptr",
+    buildStoragePrefixFromKey: () => "prefix",
+    getVideoNoteErrorMessage: (c) => `Error: ${c}`,
+    normalizeVideoNotePayload: (p) => ({ payload: p, errors: [] }),
+    calculateTorrentInfoHash: async () => "a".repeat(40),
+  };
+}
+
+const fakeFile = { name: "v.mp4", type: "video/mp4", size: 10, slice() { return this; } };
+
+async function uploadAndCaptureError(uploadImpl) {
+  const service = new S3UploadService(makeDeps(uploadImpl));
+  const errors = [];
+  service.on("uploadStatus", (d) => {
+    if (d.variant === "error") errors.push(d.message);
+  });
+  await service.uploadVideo({
+    npub: "npub1x",
+    file: fakeFile,
+    metadata: { title: "T" },
+    settings: { endpoint: "https://s3.mock", accessKeyId: "k", secretAccessKey: "s", bucket: "b" },
+    infoHash: "a".repeat(40),
+    publishVideoNote: async () => true,
+  });
+  return errors;
+}
+
+test("a CORS-like upload failure surfaces actionable CORS guidance", async () => {
+  const errors = await uploadAndCaptureError(async () => {
+    throw new TypeError("Failed to fetch");
+  });
+  const msg = errors.join(" | ");
+  assert.match(msg, /CORS/i, `expected CORS guidance, got: ${msg}`);
+  assert.match(msg, /s3\.us-east-1\.amazonaws\.com/, "names the S3 endpoint to configure");
+});
+
+test("an ordinary upload error does NOT get the CORS hint", async () => {
+  const errors = await uploadAndCaptureError(async () => {
+    throw new Error("Access Denied");
+  });
+  const msg = errors.join(" | ");
+  assert.ok(msg.includes("Access Denied"));
+  assert.doesNotMatch(msg, /CORS/i);
+});

todo/TODO_2026-06-16_r2-followups.md

@@ -44,8 +44,11 @@ Backlog from the R2 audit. Branch: `unstable` (promote down to beta/main later).
   - Note: this is for the *bitvid app* zone, separate from per-user R2 buckets.
 
 ## Open — follow-ups to the cleanup work we just shipped
-- [ ] **Generic S3 cleanup**: `deleteVideoStorage` uses `makeR2Client` (R2/path-style).
-      Add a generic-S3 path so `s3UploadService` users' deletes/edits also clean up.
+- [x] **Generic S3 cleanup** — DONE: `deleteVideoStorage` is now provider-aware
+      (uses `makeS3Client` honoring the connection's `forcePathStyle` for non-R2),
+      so generic-S3 deletes/edits clean up too. Also brought S3 to upload-failure
+      CORS-guidance parity (shared `js/utils/uploadErrorHints.js`). Generic-S3 is
+      now at functional parity with R2 for upload/publish/cleanup.
 - [ ] **All-versions delete cleanup**: delete currently cleans the target video's object;
       superseded versions from *past* edits (pre-`33c701aa`) are not retroactively cleaned.
       Consider enumerating all cached versions for a `videoRootId` on full delete.