-
-
Notifications
You must be signed in to change notification settings - Fork 852
feat: Add optional S3-based storage for conversion input/output files #514
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 issues found across 6 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/db/db.ts">
<violation number="1" location="src/db/db.ts:43">
P1: Migration to v2 skips adding `file_names.status` for legacy user_version=0 databases, leaving the column missing while marking the DB as v2.</violation>
<violation number="2" location="src/db/db.ts:43">
P1: Migration to version 2 bumps user_version but fails to create the new storage_metadata table for existing databases, leaving upgraded DBs inconsistent with the version 2 schema.</violation>
</file>
<file name="src/pages/download.tsx">
<violation number="1" location="src/pages/download.tsx:35">
P1: Removed /archive download endpoint while UI still links to it, causing 404 for tarball downloads</violation>
<violation number="2" location="src/pages/download.tsx:36">
P2: Download now buffers entire file into memory instead of streaming, causing potential memory/performance regression for large files.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/storage/LocalStorageAdapter.ts">
<violation number="1" location="src/storage/LocalStorageAdapter.ts:28">
P2: delete() now swallows all fs.unlink errors, hiding real failures and leaving undeleted files without any signal.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@C4illin implements an initial storage abstraction to support external backends (e.g. S3) for input/output files, addressing issue #511. Added pluggable storage adapters (local + S3). Default behavior remains unchanged (local filesystem). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/storage/S3StorageAdapter.ts">
<violation number="1" location="src/storage/S3StorageAdapter.ts:15">
P2: Uploads can fail on buckets with Object Ownership = Bucket owner enforced (ACLs disabled) because the code always sets `acl: "private"`, which AWS rejects unless ACLs are omitted or set to bucket-owner-full-control.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This PR adds optional support for storing conversion input and output files in S3-compatible object storage.
Summary
Introduces a simple storage adapter interface
Adds a Local and S3 storage implementation
Allows switching storage via environment variables
Stores a storage_key in the database instead of relying on filesystem paths
Updates download logic to fetch files via the storage adapter
The current implementation assumes local filesystem storage, which makes it harder to:
scale storage
use cheaper object storage (e.g. S3 / B2)
allow longer-lived output files for self-hosted deployments
This change keeps local storage as the default while allowing object storage as an opt-in.
This is an initial implementation focused on unblocking S3-based storage. If there is any need to make changes, like to discuss and update them.
fixes #511
Summary by cubic
Adds optional S3 storage for conversion input/output via a storage adapter, keeping local storage as the default. Files now use a storage_key in the database for scalable, longer-lived storage. Addresses #511.
New Features
Migration
Written for commit a814aac. Summary will update on new commits.