fix(s3, azure): stream writes instead of buffering the body

thklein commented

2026-10-02 17:19:50 +00:00

Owner

Part of basepeak-ai/bpai#1220. Every hosted tenant uses the S3 backend, and it read each written file whole into memory before uploading it.

What

S3 (pkg/client/s3.go):

Small bodies: the first part (8 MiB + 1 byte) is read up front. A body that fits is sent as one plain PutObject, so a small file costs about its own size and no part buffer is allocated.
Larger bodies: go to feature/s3/manager with 8 MiB parts and 2 in flight, about 32 MiB per write including that first chunk, instead of the file's size.
Failed uploads: a failed multipart upload is also aborted with a context that isn't cancelled, bounded at 30 s. The manager's own abort uses the request context, which is already cancelled when the client disconnects, so the parts were left behind.
Dependencies: the manager is v1.17.32, which matches the s3 and core modules already pinned, so no other module moved.

Azure (pkg/client/azure.go): UploadStream with 8 MiB blocks and 2 in flight, about 16 MiB per write. Before, the body went through io.ReadAll first.

A body that fails part-way is never stored on either backend. The S3 multipart upload is aborted, and the Azure block list is never committed.

Not in this PR

Reordering the revision bookkeeping, so that a failed write leaves revision state unchanged, was taken out after review. Doing it after the upload widens the window in which two writers with the same latestRevisionID both succeed, and makes a failed revision-info write unsafe to retry. It will come as its own PR, with a per-file lock and retry-safe commits.

Checks

go vet ./...: clean.
golangci-lint (after a cache clean): 0 issues.
go mod tidy: no diff.
go test -count=1 -race ./... without credentials, against the directory backend and an in-process fake S3: all pass.
The same suite against real Garage (dxflrs/garage:v2.1.0, path-style, the SaaS backend) and Azurite: 217 passed, 0 skipped, no objects left behind.

What the tests cover:

a 2 KB body is sent as one PutObject, with no multipart upload and under 4 MiB allocated. Against the previous s3.go this test reads 8.5 MB;
exactly 8 MiB goes out as one PutObject, and 8 MiB + 1 as a multipart upload;
20 MiB of unknown length is stored intact, with no part over 8 MiB;
a reader that fails part-way stores nothing;
a cancelled upload leaves no object and no open multipart upload.

Not run: S3 virtual-hosted style, MinIO, real Azure.

Shipping

The providers image clones this repo's main when it is built (bpai-tools/scripts/package-providers.sh), so this ships with the next bpai-tools tag after merge.

Part of basepeak-ai/bpai#1220. Every hosted tenant uses the S3 backend, and it read each written file whole into memory before uploading it. ## What **S3 (`pkg/client/s3.go`):** - **Small bodies:** the first part (8 MiB + 1 byte) is read up front. A body that fits is sent as one plain PutObject, so a small file costs about its own size and no part buffer is allocated. - **Larger bodies:** go to `feature/s3/manager` with 8 MiB parts and 2 in flight, about 32 MiB per write including that first chunk, instead of the file's size. - **Failed uploads:** a failed multipart upload is also aborted with a context that isn't cancelled, bounded at 30 s. The manager's own abort uses the request context, which is already cancelled when the client disconnects, so the parts were left behind. - **Dependencies:** the manager is v1.17.32, which matches the s3 and core modules already pinned, so no other module moved. **Azure (`pkg/client/azure.go`):** `UploadStream` with 8 MiB blocks and 2 in flight, about 16 MiB per write. Before, the body went through `io.ReadAll` first. A body that fails part-way is never stored on either backend. The S3 multipart upload is aborted, and the Azure block list is never committed. ## Not in this PR Reordering the revision bookkeeping, so that a failed write leaves revision state unchanged, was taken out after review. Doing it after the upload widens the window in which two writers with the same `latestRevisionID` both succeed, and makes a failed revision-info write unsafe to retry. It will come as its own PR, with a per-file lock and retry-safe commits. ## Checks - `go vet ./...`: clean. - `golangci-lint` (after a cache clean): 0 issues. - `go mod tidy`: no diff. - `go test -count=1 -race ./...` without credentials, against the directory backend and an in-process fake S3: all pass. - The same suite against real **Garage** (`dxflrs/garage:v2.1.0`, path-style, the SaaS backend) and **Azurite**: 217 passed, 0 skipped, no objects left behind. What the tests cover: - a 2 KB body is sent as one PutObject, with no multipart upload and under 4 MiB allocated. Against the previous `s3.go` this test reads 8.5 MB; - exactly 8 MiB goes out as one PutObject, and 8 MiB + 1 as a multipart upload; - 20 MiB of unknown length is stored intact, with no part over 8 MiB; - a reader that fails part-way stores nothing; - a cancelled upload leaves no object and no open multipart upload. Not run: S3 virtual-hosted style, MinIO, real Azure. ## Shipping The providers image clones this repo's `main` when it is built (`bpai-tools/scripts/package-providers.sh`), so this ships with the next bpai-tools tag after merge.

thklein added 3 commits

2026-10-02 17:19:50 +00:00

fix(s3): stream writes instead of buffering the body 4bb3e3290e

WriteFile read an unknown-length body whole with io.ReadAll before
PutObject, so every large file cost its full size in the daemon. Upload
through the s3 transfer manager (8 MiB parts, 2 in flight, ~24 MiB per
write) instead; small bodies still go out as a single PutObject.

The manager aborts a failed multipart upload with the request context,
which is already cancelled when the client disconnects, so parts were
left behind. Abort again with a non-cancelled context.

Refs basepeak-ai/bpai#1220

fix(azure): stream writes instead of buffering the body 0fdba21a80

WriteFile read the whole body with io.ReadAll and then handed a bytes
reader to UploadStream. Pass the body straight to UploadStream with 8 MiB
blocks and 2 in flight, so a write costs ~16 MiB whatever its size.

Refs basepeak-ai/bpai#1220

fix(revisions): leave revision state unchanged when a write fails

test / test (pull_request) Has been cancelled

Details

111ed92e16

WriteFile bumped and stored the revision info before reading the new
content. A body that failed part-way stored nothing, but CurrentID had
moved, so a retry with the same latestRevisionID got a spurious
conflict (and IfNotExists failed for a file that was never written).

Keep the conflict check first, snapshot the current content into
revision N+1, store the new content, and only then write the revision
info. On failure the snapshot is deleted; it sits beyond CurrentID, so
it is invisible either way. The directory provider stages the temp file
before taking the snapshot, so a failed body there touches no revision
state at all.

Refs basepeak-ai/bpai#1220

thklein force-pushed feat/stream-s3-azure-writes from 111ed92e16

test / test (pull_request) Has been cancelled

Details

to 96d31bce6a

test / test (pull_request) Successful in 4m39s

Details

2026-10-02 17:27:53 +00:00

Compare

thklein changed title from ~~fix(s3, azure): stream writes, and keep revisions intact on failure~~ to fix(s3, azure): stream writes instead of buffering the body

2026-10-02 17:28:36 +00:00

thklein commented

2026-10-02 17:28:36 +00:00

Author

Owner

Review (two fresh-context reviewers): outcome

Fixed (96d31bc): every streamed S3 write used to allocate a full 8 MiB part buffer, even for tiny files: ~8.5 MB for a 2 KB write. A body that fits in one part is now sent as a plain PutObject. There is a test for that, plus tests at the exactly-part-size and part-size+1 boundaries, and for a reader that fails inside the first part.

Split out: the revision reordering (formerly 111ed92). Review found it made two concurrent writers with the same latestRevisionID both succeed, and one write could be lost with no conflict (verified; main returns a conflict). It also made a failed revision-info write unsafe to retry, and a cancel after commit could delete the only copy of the old content. That work needs a per-file lock and retry-safe commits, so it becomes a follow-up PR.

Confirmed safe:

a reader failing mid-multipart never completes the upload, and the abort reaches the server;
the single-PutObject path never sends a partial body;
Azure never commits a block list after a reader error;
the directory provider's temp-file cleanup runs on every path;
the conflict and not-found error texts callers parse are unchanged.

Pre-existing, noted for the revisions follow-up:

a crash-left hidden revision N+1 survives DeleteFile and stays readable through GetRevision (erasure);
createRevision=false skips the latestRevisionID check entirely;
there is no logging in the provider, so best-effort failures are invisible.

## Review (two fresh-context reviewers): outcome **Fixed (`96d31bc`):** every streamed S3 write used to allocate a full 8 MiB part buffer, even for tiny files: ~8.5 MB for a 2 KB write. A body that fits in one part is now sent as a plain PutObject. There is a test for that, plus tests at the exactly-part-size and part-size+1 boundaries, and for a reader that fails inside the first part. **Split out:** the revision reordering (formerly `111ed92`). Review found it made two concurrent writers with the same `latestRevisionID` both succeed, and one write could be lost with no conflict (verified; main returns a conflict). It also made a failed revision-info write unsafe to retry, and a cancel after commit could delete the only copy of the old content. That work needs a per-file lock and retry-safe commits, so it becomes a follow-up PR. **Confirmed safe:** - a reader failing mid-multipart never completes the upload, and the abort reaches the server; - the single-PutObject path never sends a partial body; - Azure never commits a block list after a reader error; - the directory provider's temp-file cleanup runs on every path; - the conflict and not-found error texts callers parse are unchanged. **Pre-existing, noted for the revisions follow-up:** - a crash-left hidden revision `N+1` survives `DeleteFile` and stays readable through `GetRevision` (erasure); - `createRevision=false` skips the `latestRevisionID` check entirely; - there is no logging in the provider, so best-effort failures are invisible.

thklein merged commit 4bdd4bfb50 into main

2026-10-02 18:06:53 +00:00

thklein deleted branch feat/stream-s3-azure-writes

2026-10-02 18:06:53 +00:00

thklein referenced this pull request from a commit

2026-10-02 18:06:54 +00:00

fix(s3, azure): stream writes instead of buffering the body (#27)

thklein referenced this pull request

2026-10-02 19:26:59 +00:00

fix(revisions): leave revision state unchanged when a write fails #28

thklein referenced this pull request

2026-10-03 05:04:30 +00:00