Various upload improvements#1012
Conversation
169e5a9 to
4c02732
Compare
db56d5c to
01f9643
Compare
In #1012, I'm updating the Go version to 1.25 to be able to use the experimental `synctest` package, but one of our linters isn't compatible. I opened a PR (linked in the code), but I don't want to have to wait for the maintainer (there's a PR that's 2 weeks old with no comments). So this PR uses my fork instead.
01f9643 to
1177c8d
Compare
1177c8d to
f5722c6
Compare
|
|
||
| if db != nil && db.Sleeping { | ||
| return fmt.Errorf("Your DB might be archived. Please run `turso group unarchive " + db.Group + "` to unarchive it") | ||
| return fmt.Errorf("Your DB might be archived. Please run `turso group unarchive %s` to unarchive it", db.Group) |
|
@copilot are you there? |
|
@LeMikaelF I've opened a new pull request, #1016, to work on those changes. Once the pull request is ready, I'll request review from you. |
f5722c6 to
be8d425
Compare
02a0cb5 to
97876ec
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces comprehensive improvements to the multipart upload flow for database files, transitioning to always use multipart uploads with enhanced reliability and progress tracking.
Changes:
- Implemented chunk-level retry logic with exponential backoff for failed uploads
- Added token refresh mechanism between chunks and before finalization to handle long-running uploads
- Enhanced progress tracking with time-based (2s) and byte-based (50MB) update thresholds
- Migrated to v2 upload endpoints with upload_id support and removed single-part upload path
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updates Go version requirement to 1.25 (critical: this version doesn't exist) |
| internal/turso/turso.go | Adds SetToken method to support token refresh |
| internal/turso/tursoServer.go | Core upload logic with retry mechanism, token refresh, and improved progress tracking |
| internal/turso/tursoServer_test.go | Updates tests for v2 API and adds comprehensive progress reader tests using synctest |
| internal/turso/databases.go | Refactors to use TokenProvider pattern and removes multipart flag (always multipart now) |
| internal/cmd/db_create.go | Removes multipart flag parameter |
| internal/cmd/db_import.go | Removes multipart flag declaration |
| internal/cmd/db_shell.go | Improves error message formatting |
| internal/cmd/group_flag.go | Adds validation spinner with file size display and refactors DB seed handling |
| internal/cmd/group_flag_test.go | New test file for database validation logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log.Printf("[upload] Chunk %d: exhausted all %d retries, giving up", ctx.chunkID, maxRetries+1) | ||
| } | ||
| return chunkUploadResult{}, fmt.Errorf("failed after %d retries: %w", maxRetries+1, lastErr) |
There was a problem hiding this comment.
The retry count message is misleading. When maxRetries is 15, the loop runs 16 times (attempts 0-15), but the error message says "failed after 16 retries" which should be "failed after 15 retries" (since attempt 0 is the initial try, not a retry). Consider changing to "failed after %d attempts" or adjusting the calculation to show actual retry count.
| log.Printf("[upload] Chunk %d: exhausted all %d retries, giving up", ctx.chunkID, maxRetries+1) | |
| } | |
| return chunkUploadResult{}, fmt.Errorf("failed after %d retries: %w", maxRetries+1, lastErr) | |
| log.Printf("[upload] Chunk %d: exhausted all %d attempts, giving up", ctx.chunkID, maxRetries+1) | |
| } | |
| return chunkUploadResult{}, fmt.Errorf("failed after %d attempts: %w", maxRetries+1, lastErr) |
Summary
This PR contains various improvements to the multipart upload flow. I realize that it's a lot in the same PR, sorry about that.
/finalize. This was necessary because the new flow can include several requests over more than an hour.TURSO_DEBUG_UPLOAD=1environment variable. I wasn't able to use the existing--debugflag, because that one logs the bodies of the PUT requests, and they can contain control characters that will wreck your terminal.Testing
I uploaded a very small databases (few KB), one that was ~100MB, and one that was 1GB. There are also some automated tests in the PR.
Failing CI test
The integration tests are failing in the CI. It's because multipart upload isn't deployed to us-east-1 yet. I changed the integration tests to use
eu-north-1. You can see a passing run in that PR (https://github.com/tursodatabase/tursotest/pull/93).AI use
The code was mostly written by Claude, but I iterated a lot with it.