| 1 | diff --git a/internal/blob/blob.go b/internal/blob/blob.go |
| 2 | index 134df32..6e79cbc 100644 |
| 3 | --- a/internal/blob/blob.go |
| 4 | +++ b/internal/blob/blob.go |
| 5 | @@ -93,6 +93,13 @@ type Service struct { |
| 6 | // before quotaMu; network bodies are never read while either is held. |
| 7 | multipartMu sync.Mutex |
| 8 | activeParts map[string]int |
| 9 | + // multipartFinalizing prevents another chunk from changing a complete |
| 10 | + // session while its assembled file is being hashed. Hashing is deliberately |
| 11 | + // outside multipartMu because the upload may be as large as 1 GiB. |
| 12 | + multipartFinalizing map[string]bool |
| 13 | + // verifyMultipartHook is used by package tests to pause finalization |
| 14 | + // without allocating a GiB fixture. It is nil in production. |
| 15 | + verifyMultipartHook func(string, string, int64) error |
| 16 | } |
| 17 | |
| 18 | var ErrBlocked = errors.New("blocked: this blob was removed by a moderator") |
| 19 | @@ -159,7 +166,7 @@ func New(ctx context.Context, config Config) (*Service, error) { |
| 20 | return net.DefaultResolver.LookupIP(ctx, "ip", host) |
| 21 | } |
| 22 | } |
| 23 | - s := &Service{config: config, root: filepath.Join(config.Root, "blobs"), store: config.Store, resolve: resolve, activeParts: make(map[string]int)} |
| 24 | + s := &Service{config: config, root: filepath.Join(config.Root, "blobs"), store: config.Store, resolve: resolve, activeParts: make(map[string]int), multipartFinalizing: make(map[string]bool)} |
| 25 | if err := s.reconcileMultipart(ctx); err != nil { |
| 26 | return nil, err |
| 27 | } |
| 28 | @@ -430,8 +437,8 @@ func (s *Service) uploadHead(w http.ResponseWriter, r *http.Request) { |
| 29 | return |
| 30 | } |
| 31 | length, err := strconv.ParseInt(rawLength, 10, 64) |
| 32 | - if err != nil || length <= 0 { |
| 33 | - s.fail(w, http.StatusBadRequest, errors.New("invalid: X-Content-Length must be a positive integer")) |
| 34 | + if err != nil || length < 0 { |
| 35 | + s.fail(w, http.StatusBadRequest, errors.New("invalid: X-Content-Length must be a nonnegative integer")) |
| 36 | return |
| 37 | } |
| 38 | pubkey, err := s.authorize(r, ActionUpload) |
| 39 | diff --git a/internal/blob/multipart.go b/internal/blob/multipart.go |
| 40 | index 8b2a480..0b744e2 100644 |
| 41 | --- a/internal/blob/multipart.go |
| 42 | +++ b/internal/blob/multipart.go |
| 43 | @@ -285,46 +285,78 @@ func (s *Service) releaseMultipart(id string) { |
| 44 | |
| 45 | func (s *Service) commitMultipart(ctx context.Context, part multipartRequest, chunk string) (Blob, bool, bool, error) { |
| 46 | s.multipartMu.Lock() |
| 47 | - defer s.multipartMu.Unlock() |
| 48 | + if s.multipartFinalizing[part.id] { |
| 49 | + s.multipartMu.Unlock() |
| 50 | + return Blob{}, false, false, multipartFailure(http.StatusConflict, "multipart upload is being finalized") |
| 51 | + } |
| 52 | // Recheck policy after streaming, including uploads whose next chunk does |
| 53 | // not complete the file. Policy may have changed while the body arrived. |
| 54 | s.quotaMu.Lock() |
| 55 | admissionErr := s.multipartAdmission(ctx, part) |
| 56 | s.quotaMu.Unlock() |
| 57 | if admissionErr != nil { |
| 58 | + s.multipartMu.Unlock() |
| 59 | return Blob{}, false, false, admissionErr |
| 60 | } |
| 61 | candidate := uploadCandidate{hash: part.hash, size: part.length, typ: part.typ, uploader: part.uploader, reservation: part.id} |
| 62 | if existing, err := s.lookup(ctx, part.hash); err == nil { |
| 63 | if existing.Size != part.length { |
| 64 | + s.multipartMu.Unlock() |
| 65 | return Blob{}, false, false, ErrMultipartConflict |
| 66 | } |
| 67 | entry, _, err := s.installUpload(ctx, candidate) |
| 68 | + s.multipartMu.Unlock() |
| 69 | return entry, false, err == nil, err |
| 70 | } else if !errors.Is(err, sql.ErrNoRows) { |
| 71 | + s.multipartMu.Unlock() |
| 72 | return Blob{}, false, false, err |
| 73 | } |
| 74 | ranges, err := s.multipartRanges(ctx, part) |
| 75 | if err != nil { |
| 76 | + s.multipartMu.Unlock() |
| 77 | return Blob{}, false, false, err |
| 78 | } |
| 79 | candidate.path = s.multipartPath(part.id) |
| 80 | if err := writeMultipartChunk(candidate.path, chunk, part.offset); err != nil { |
| 81 | _ = s.discardMultipart(ctx, part.id) |
| 82 | + s.multipartMu.Unlock() |
| 83 | return Blob{}, false, false, err |
| 84 | } |
| 85 | if err := s.recordMultipartRanges(ctx, part.id, ranges); err != nil { |
| 86 | _ = s.discardMultipart(ctx, part.id) |
| 87 | + s.multipartMu.Unlock() |
| 88 | return Blob{}, false, false, err |
| 89 | } |
| 90 | if len(ranges) != 1 || ranges[0].start != 0 || ranges[0].end != part.length { |
| 91 | + s.multipartMu.Unlock() |
| 92 | return Blob{}, false, false, nil |
| 93 | } |
| 94 | - if err := verifyMultipart(candidate.path, part.hash, part.length); err != nil { |
| 95 | - return Blob{}, false, false, errors.Join(err, s.discardMultipart(ctx, part.id)) |
| 96 | + // Keep the session reserved while hashing, but let unrelated multipart |
| 97 | + // sessions and ordinary uploads proceed. The finalizing marker makes this |
| 98 | + // small state transition safe without holding the global mutex for the hash. |
| 99 | + s.multipartFinalizing[part.id] = true |
| 100 | + s.multipartMu.Unlock() |
| 101 | + verify := verifyMultipart |
| 102 | + if s.verifyMultipartHook != nil { |
| 103 | + verify = s.verifyMultipartHook |
| 104 | + } |
| 105 | + if err := verify(candidate.path, part.hash, part.length); err != nil { |
| 106 | + s.multipartMu.Lock() |
| 107 | + delete(s.multipartFinalizing, part.id) |
| 108 | + // A canceled request must not throw away an otherwise complete session: |
| 109 | + // the caller can retry finalization without retransmitting its chunks. |
| 110 | + var cleanupErr error |
| 111 | + if !errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded) { |
| 112 | + cleanupErr = s.discardMultipart(ctx, part.id) |
| 113 | + } |
| 114 | + s.multipartMu.Unlock() |
| 115 | + return Blob{}, false, false, errors.Join(err, cleanupErr) |
| 116 | } |
| 117 | + s.multipartMu.Lock() |
| 118 | entry, created, err := s.installUpload(ctx, candidate) |
| 119 | cleanupErr := s.discardMultipart(ctx, part.id) |
| 120 | + delete(s.multipartFinalizing, part.id) |
| 121 | + s.multipartMu.Unlock() |
| 122 | return entry, created, err == nil && cleanupErr == nil, errors.Join(err, cleanupErr) |
| 123 | } |
| 124 | |
| 125 | diff --git a/internal/blob/multipart_review_test.go b/internal/blob/multipart_review_test.go |
| 126 | index cc91b2f..594cef1 100644 |
| 127 | --- a/internal/blob/multipart_review_test.go |
| 128 | +++ b/internal/blob/multipart_review_test.go |
| 129 | @@ -3,11 +3,13 @@ package blob |
| 130 | import ( |
| 131 | "context" |
| 132 | "crypto/sha256" |
| 133 | + "database/sql" |
| 134 | "encoding/hex" |
| 135 | "errors" |
| 136 | "io" |
| 137 | "net/http" |
| 138 | "net/http/httptest" |
| 139 | + "os" |
| 140 | "path/filepath" |
| 141 | "strings" |
| 142 | "sync" |
| 143 | @@ -105,6 +107,222 @@ func TestMultipartReviewStalledBodyDoesNotBlockOrdinaryUpload(t *testing.T) { |
| 144 | } |
| 145 | } |
| 146 | |
| 147 | +func TestMultipartReviewStalledHashDoesNotBlockOrdinaryUpload(t *testing.T) { |
| 148 | + s := testService(t) |
| 149 | + body := "hash-paused" |
| 150 | + digest := sha256.Sum256([]byte(body)) |
| 151 | + sha := hex.EncodeToString(digest[:]) |
| 152 | + started := make(chan struct{}) |
| 153 | + release := make(chan struct{}) |
| 154 | + s.verifyMultipartHook = func(path, hash string, size int64) error { |
| 155 | + close(started) |
| 156 | + <-release |
| 157 | + return verifyMultipart(path, hash, size) |
| 158 | + } |
| 159 | + result := make(chan *httptest.ResponseRecorder, 1) |
| 160 | + r := multipartReviewRequest(sha, int64(len(body)), 0, body) |
| 161 | + go func() { result <- record(s.Handler(), r) }() |
| 162 | + select { |
| 163 | + case <-started: |
| 164 | + case <-time.After(time.Second): |
| 165 | + t.Fatal("multipart hash was not started") |
| 166 | + } |
| 167 | + ordinary := make(chan error, 1) |
| 168 | + go func() { |
| 169 | + _, err := s.Put(context.Background(), PutOptions{Reader: strings.NewReader("ordinary"), Type: "text/plain", Uploader: "other"}) |
| 170 | + ordinary <- err |
| 171 | + }() |
| 172 | + select { |
| 173 | + case err := <-ordinary: |
| 174 | + if err != nil { |
| 175 | + t.Fatalf("ordinary upload while multipart hash paused: %v", err) |
| 176 | + } |
| 177 | + case <-time.After(time.Second): |
| 178 | + t.Fatal("ordinary upload blocked by multipart hashing") |
| 179 | + } |
| 180 | + close(release) |
| 181 | + select { |
| 182 | + case response := <-result: |
| 183 | + if response.Code != http.StatusCreated { |
| 184 | + t.Fatalf("multipart completion status = %d: %s", response.Code, response.Body.String()) |
| 185 | + } |
| 186 | + case <-time.After(time.Second): |
| 187 | + t.Fatal("multipart hash did not finish") |
| 188 | + } |
| 189 | +} |
| 190 | + |
| 191 | +func TestMultipartReviewStalledHashDoesNotBlockAnotherSession(t *testing.T) { |
| 192 | + s := testService(t) |
| 193 | + body := "hash-paused" |
| 194 | + digest := sha256.Sum256([]byte(body)) |
| 195 | + sha := hex.EncodeToString(digest[:]) |
| 196 | + started := make(chan struct{}) |
| 197 | + release := make(chan struct{}) |
| 198 | + var unblock sync.Once |
| 199 | + defer unblock.Do(func() { close(release) }) |
| 200 | + s.verifyMultipartHook = func(path, hash string, size int64) error { |
| 201 | + close(started) |
| 202 | + <-release |
| 203 | + return verifyMultipart(path, hash, size) |
| 204 | + } |
| 205 | + first := make(chan *httptest.ResponseRecorder, 1) |
| 206 | + go func() { first <- record(s.Handler(), multipartReviewRequest(sha, int64(len(body)), 0, body)) }() |
| 207 | + select { |
| 208 | + case <-started: |
| 209 | + case <-time.After(5 * time.Second): |
| 210 | + t.Fatal("multipart hash did not start") |
| 211 | + } |
| 212 | + // A different session must reserve space and commit a partial chunk while |
| 213 | + // the first session hashes. Ordinary PUT does not take multipartMu and |
| 214 | + // would not detect the tenant-wide finalization lock regression. |
| 215 | + secondBody := strings.Repeat("b", 70000) |
| 216 | + secondDigest := sha256.Sum256([]byte(secondBody + "remaining")) |
| 217 | + second := make(chan *httptest.ResponseRecorder, 1) |
| 218 | + go func() { |
| 219 | + second <- record(s.Handler(), multipartReviewRequest(hex.EncodeToString(secondDigest[:]), int64(len(secondBody)+len("remaining")), 0, secondBody)) |
| 220 | + }() |
| 221 | + select { |
| 222 | + case response := <-second: |
| 223 | + if response.Code != http.StatusNoContent { |
| 224 | + t.Fatalf("second session = %d: %s", response.Code, response.Body.String()) |
| 225 | + } |
| 226 | + case <-time.After(5 * time.Second): |
| 227 | + t.Fatal("another multipart session blocked by hashing") |
| 228 | + } |
| 229 | + unblock.Do(func() { close(release) }) |
| 230 | + select { |
| 231 | + case response := <-first: |
| 232 | + if response.Code != http.StatusCreated { |
| 233 | + t.Fatalf("finalizing session = %d: %s", response.Code, response.Body.String()) |
| 234 | + } |
| 235 | + case <-time.After(5 * time.Second): |
| 236 | + t.Fatal("finalizing session did not finish") |
| 237 | + } |
| 238 | +} |
| 239 | + |
| 240 | +func TestMultipartReviewFinalizingSessionRejectsConcurrentChunk(t *testing.T) { |
| 241 | + s := testService(t) |
| 242 | + body := "same-session" |
| 243 | + digest := sha256.Sum256([]byte(body)) |
| 244 | + sha := hex.EncodeToString(digest[:]) |
| 245 | + started := make(chan struct{}) |
| 246 | + release := make(chan struct{}) |
| 247 | + s.verifyMultipartHook = func(path, hash string, size int64) error { |
| 248 | + close(started) |
| 249 | + <-release |
| 250 | + return verifyMultipart(path, hash, size) |
| 251 | + } |
| 252 | + first := make(chan *httptest.ResponseRecorder, 1) |
| 253 | + go func() { first <- record(s.Handler(), multipartReviewRequest(sha, int64(len(body)), 0, body)) }() |
| 254 | + select { |
| 255 | + case <-started: |
| 256 | + case <-time.After(time.Second): |
| 257 | + t.Fatal("multipart hash was not started") |
| 258 | + } |
| 259 | + second := record(s.Handler(), multipartReviewRequest(sha, int64(len(body)), 0, body)) |
| 260 | + if second.Code != http.StatusConflict { |
| 261 | + t.Fatalf("concurrent same-session chunk status = %d: %s", second.Code, second.Body.String()) |
| 262 | + } |
| 263 | + close(release) |
| 264 | + if response := <-first; response.Code != http.StatusCreated { |
| 265 | + t.Fatalf("finalizing upload status = %d: %s", response.Code, response.Body.String()) |
| 266 | + } |
| 267 | +} |
| 268 | + |
| 269 | +func TestMultipartReviewCleanupDoesNotRemoveActiveFinalization(t *testing.T) { |
| 270 | + s := testService(t) |
| 271 | + body := "cleanup-paused" |
| 272 | + digest := sha256.Sum256([]byte(body)) |
| 273 | + sha := hex.EncodeToString(digest[:]) |
| 274 | + started := make(chan struct{}) |
| 275 | + release := make(chan struct{}) |
| 276 | + s.verifyMultipartHook = func(path, hash string, size int64) error { |
| 277 | + close(started) |
| 278 | + <-release |
| 279 | + return verifyMultipart(path, hash, size) |
| 280 | + } |
| 281 | + result := make(chan *httptest.ResponseRecorder, 1) |
| 282 | + go func() { result <- record(s.Handler(), multipartReviewRequest(sha, int64(len(body)), 0, body)) }() |
| 283 | + select { |
| 284 | + case <-started: |
| 285 | + case <-time.After(time.Second): |
| 286 | + t.Fatal("multipart hash was not started") |
| 287 | + } |
| 288 | + if _, err := s.store.DB().Exec("UPDATE multipart_uploads SET last_seen=0"); err != nil { |
| 289 | + t.Fatal(err) |
| 290 | + } |
| 291 | + if err := s.CleanupMultipart(context.Background()); err != nil { |
| 292 | + t.Fatal(err) |
| 293 | + } |
| 294 | + id := multipartID(sha, "uploader") |
| 295 | + if _, err := os.Stat(s.multipartPath(id)); err != nil { |
| 296 | + t.Fatalf("active multipart file removed during cleanup: %v", err) |
| 297 | + } |
| 298 | + var found int |
| 299 | + if err := s.store.DB().QueryRow("SELECT COUNT(*) FROM multipart_uploads WHERE id=?", id).Scan(&found); err != nil { |
| 300 | + t.Fatal(err) |
| 301 | + } |
| 302 | + if found != 1 { |
| 303 | + t.Fatal("active multipart reservation removed during cleanup") |
| 304 | + } |
| 305 | + close(release) |
| 306 | + if response := <-result; response.Code != http.StatusCreated { |
| 307 | + t.Fatalf("finalizing upload status = %d: %s", response.Code, response.Body.String()) |
| 308 | + } |
| 309 | +} |
| 310 | + |
| 311 | +func TestMultipartReviewPolicyRecheckedAfterHash(t *testing.T) { |
| 312 | + s := testService(t) |
| 313 | + body := "policy-after-hash" |
| 314 | + digest := sha256.Sum256([]byte(body)) |
| 315 | + sha := hex.EncodeToString(digest[:]) |
| 316 | + started := make(chan struct{}) |
| 317 | + release := make(chan struct{}) |
| 318 | + s.verifyMultipartHook = func(path, hash string, size int64) error { |
| 319 | + close(started) |
| 320 | + <-release |
| 321 | + return verifyMultipart(path, hash, size) |
| 322 | + } |
| 323 | + s.config.Limits = func() Limits { return Limits{MaxFileBytes: int64(len(body))} } |
| 324 | + result := make(chan *httptest.ResponseRecorder, 1) |
| 325 | + go func() { result <- record(s.Handler(), multipartReviewRequest(sha, int64(len(body)), 0, body)) }() |
| 326 | + <-started |
| 327 | + s.config.Limits = func() Limits { return Limits{MaxFileBytes: int64(len(body) - 1)} } |
| 328 | + close(release) |
| 329 | + if response := <-result; response.Code != http.StatusRequestEntityTooLarge { |
| 330 | + t.Fatalf("policy-changed upload status = %d: %s", response.Code, response.Body.String()) |
| 331 | + } |
| 332 | + if _, _, err := s.Get(context.Background(), sha); !errors.Is(err, sql.ErrNoRows) { |
| 333 | + t.Fatalf("policy-rejected upload lookup = %v", err) |
| 334 | + } |
| 335 | +} |
| 336 | + |
| 337 | +func TestMultipartReviewCanceledHashCanBeRetried(t *testing.T) { |
| 338 | + s := testService(t) |
| 339 | + body := "cancel-and-retry" |
| 340 | + digest := sha256.Sum256([]byte(body)) |
| 341 | + sha := hex.EncodeToString(digest[:]) |
| 342 | + started := make(chan struct{}) |
| 343 | + release := make(chan struct{}) |
| 344 | + s.verifyMultipartHook = func(path, hash string, size int64) error { |
| 345 | + close(started) |
| 346 | + <-release |
| 347 | + return verifyMultipart(path, hash, size) |
| 348 | + } |
| 349 | + ctx, cancel := context.WithCancel(context.Background()) |
| 350 | + r := multipartReviewRequest(sha, int64(len(body)), 0, body).WithContext(ctx) |
| 351 | + result := make(chan *httptest.ResponseRecorder, 1) |
| 352 | + go func() { result <- record(s.Handler(), r) }() |
| 353 | + <-started |
| 354 | + cancel() |
| 355 | + close(release) |
| 356 | + <-result |
| 357 | + s.verifyMultipartHook = nil |
| 358 | + if response := record(s.Handler(), multipartReviewRequest(sha, int64(len(body)), 0, body)); response.Code != http.StatusCreated { |
| 359 | + t.Fatalf("retry after canceled hash status = %d: %s", response.Code, response.Body.String()) |
| 360 | + } |
| 361 | +} |
| 362 | + |
| 363 | type multipartBlockingBody struct { |
| 364 | io.Reader |
| 365 | started chan<- struct{} |
| 366 | diff --git a/internal/blob/upload_head_test.go b/internal/blob/upload_head_test.go |
| 367 | new file mode 100644 |
| 368 | index 0000000..66a64cd |
| 369 | --- /dev/null |
| 370 | +++ b/internal/blob/upload_head_test.go |
| 371 | @@ -0,0 +1,71 @@ |
| 372 | +package blob |
| 373 | + |
| 374 | +import ( |
| 375 | + "context" |
| 376 | + "crypto/sha256" |
| 377 | + "encoding/hex" |
| 378 | + "net/http" |
| 379 | + "net/http/httptest" |
| 380 | + "strings" |
| 381 | + "testing" |
| 382 | +) |
| 383 | + |
| 384 | +func TestUploadHeadAcceptsEmptyFileAtFullQuota(t *testing.T) { |
| 385 | + s := testService(t) |
| 386 | + s.config.Limits = func() Limits { return Limits{MaxFileBytes: 2, UserStorageBytes: 2} } |
| 387 | + if _, err := s.Put(context.Background(), PutOptions{Reader: strings.NewReader("ab"), Type: "text/plain", Uploader: testUploader}); err != nil { |
| 388 | + t.Fatal(err) |
| 389 | + } |
| 390 | + digest := sha256.Sum256(nil) |
| 391 | + hash := hex.EncodeToString(digest[:]) |
| 392 | + for _, phase := range []string{"before upload", "after upload"} { |
| 393 | + req := httptest.NewRequest(http.MethodHead, "https://relay.test/upload", nil) |
| 394 | + req.Header.Set("X-SHA-256", hash) |
| 395 | + req.Header.Set("X-Content-Length", "0") |
| 396 | + req.Header.Set("X-Content-Type", "application/octet-stream") |
| 397 | + response := record(s.Handler(), req) |
| 398 | + if response.Code != http.StatusOK { |
| 399 | + t.Fatalf("%s: empty preflight = %d %s", phase, response.Code, response.Body.String()) |
| 400 | + } |
| 401 | + if phase == "before upload" { |
| 402 | + upload := record(s.Handler(), httptest.NewRequest(http.MethodPut, "https://relay.test/"+hash, http.NoBody)) |
| 403 | + if upload.Code != http.StatusCreated { |
| 404 | + t.Fatalf("empty PUT = %d %s", upload.Code, upload.Body.String()) |
| 405 | + } |
| 406 | + } |
| 407 | + } |
| 408 | + stored, file, err := s.Get(context.Background(), hash) |
| 409 | + if err != nil { |
| 410 | + t.Fatal(err) |
| 411 | + } |
| 412 | + defer file.Close() |
| 413 | + if stored.Size != 0 { |
| 414 | + t.Fatalf("empty stored blob = %+v, %v", stored, err) |
| 415 | + } |
| 416 | + usage, err := s.quotaUsage(context.Background(), testUploader) |
| 417 | + if err != nil || usage != 2 { |
| 418 | + t.Fatalf("empty file changed usage: %d, %v", usage, err) |
| 419 | + } |
| 420 | +} |
| 421 | + |
| 422 | +func TestUploadHeadRequiresNonnegativeLength(t *testing.T) { |
| 423 | + for _, tc := range []struct { |
| 424 | + value string |
| 425 | + code int |
| 426 | + }{ |
| 427 | + {"", http.StatusLengthRequired}, {"-1", http.StatusBadRequest}, |
| 428 | + {"no", http.StatusBadRequest}, {"9223372036854775808", http.StatusBadRequest}, |
| 429 | + {"0", http.StatusOK}, {"1", http.StatusOK}, |
| 430 | + } { |
| 431 | + t.Run(tc.value, func(t *testing.T) { |
| 432 | + s := testService(t) |
| 433 | + req := httptest.NewRequest(http.MethodHead, "https://relay.test/upload", nil) |
| 434 | + req.Header.Set("X-SHA-256", strings.Repeat("a", 64)) |
| 435 | + req.Header.Set("X-Content-Type", "text/plain") |
| 436 | + req.Header.Set("X-Content-Length", tc.value) |
| 437 | + if response := record(s.Handler(), req); response.Code != tc.code { |
| 438 | + t.Fatalf("length %q = %d, want %d", tc.value, response.Code, tc.code) |
| 439 | + } |
| 440 | + }) |
| 441 | + } |
| 442 | +} |