-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(sdk): apply the upload headers the API returns with a file upload link #1870
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
Changes from all commits
def2bd7
105242e
5d27b1f
bfc487d
1cfbb7c
8f4c744
5085b81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| "e2b": patch | ||
| "@e2b/python-sdk": patch | ||
| --- | ||
|
|
||
| Apply the request headers the API returns with a template layer-file upload link. Azure Blob Storage requires `x-ms-blob-type` on the upload request, which its signed URL cannot carry, so `COPY` instructions failed on Azure-backed clusters. GCS- and S3-backed clusters return no headers and are unaffected. |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,7 +1,7 @@ | ||||||||
| import asyncio | ||||||||
| import os | ||||||||
| from types import TracebackType | ||||||||
| from typing import Callable, Optional, List, Union | ||||||||
| from typing import Callable, Dict, Optional, List, Union | ||||||||
|
|
||||||||
| import httpx | ||||||||
| from pyqwest import HTTPTransport | ||||||||
|
|
@@ -115,6 +115,8 @@ async def upload_file( | |||||||
| resolve_symlinks: bool, | ||||||||
| gzip: bool, | ||||||||
| stack_trace: Optional[TracebackType], | ||||||||
| *, | ||||||||
| headers: Optional[Dict[str, str]] = None, | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. T-3a: same as the sync variant — make the new optional keyword-only.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 105242e — sync now merges API headers under its own |
||||||||
| request_timeout: Optional[float] = None, | ||||||||
| ): | ||||||||
| # Uploading a large build-context archive can take far longer than the 60s | ||||||||
|
|
@@ -152,14 +154,18 @@ async def upload_file( | |||||||
| ) | ||||||||
| ), | ||||||||
| ) as client: | ||||||||
| # Stream the archive from disk via an async iterator. The | ||||||||
| # explicit Content-Length suppresses chunked transfer | ||||||||
| # encoding, which S3 presigned URLs reject; reqwest keeps the | ||||||||
| # Content-Length framing for the streamed body. | ||||||||
| # API-returned headers applied as given, but Content-Length stays ours — explicit so S3 presigned URLs see no chunked encoding. | ||||||||
| response = await client.put( | ||||||||
| url, | ||||||||
| content=aiter_io_chunks(tar_file), | ||||||||
| headers={"Content-Length": str(size)}, | ||||||||
| headers={ | ||||||||
| **{ | ||||||||
| k: v | ||||||||
| for k, v in (headers or {}).items() | ||||||||
| if k.lower() != "content-length" | ||||||||
| }, | ||||||||
| "Content-Length": str(size), | ||||||||
| }, | ||||||||
| ) | ||||||||
| response.raise_for_status() | ||||||||
| finally: | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||||||
| import os | ||||||||
| import time | ||||||||
| from types import TracebackType | ||||||||
| from typing import Callable, Optional, List, Union | ||||||||
| from typing import Callable, Dict, Optional, List, Union | ||||||||
|
|
||||||||
| import httpx | ||||||||
| from pyqwest import SyncHTTPTransport | ||||||||
|
|
@@ -113,6 +114,8 @@ def upload_file( | |||||||
| resolve_symlinks: bool, | ||||||||
| gzip: bool, | ||||||||
| stack_trace: Optional[TracebackType], | ||||||||
| *, | ||||||||
| headers: Optional[Dict[str, str]] = None, | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. T-3a (optionals are keyword-only, enforced by a bare
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 105242e — sync now merges API headers under its own |
||||||||
| request_timeout: Optional[float] = None, | ||||||||
| ): | ||||||||
| # Uploading a large build-context archive can take far longer than the 60s | ||||||||
|
|
@@ -127,6 +130,7 @@ def upload_file( | |||||||
| tar_file = tar_file_stream( | ||||||||
| file_name, context_path, ignore_patterns, resolve_symlinks, gzip | ||||||||
| ) | ||||||||
| size = os.fstat(tar_file.fileno()).st_size | ||||||||
| try: | ||||||||
| # Through the pyqwest adapter the upload timeout is a | ||||||||
| # whole-request deadline for the entire transfer, not a per-write | ||||||||
|
|
@@ -148,11 +152,19 @@ def upload_file( | |||||||
| ) | ||||||||
| ), | ||||||||
| ) as client: | ||||||||
| # httpx streams the archive from disk in chunks and sets | ||||||||
| # Content-Length from the file size—S3 presigned URLs reject | ||||||||
| # chunked transfer encoding, and reqwest keeps the | ||||||||
| # Content-Length framing for the streamed body. | ||||||||
| response = client.put(url, content=tar_file) | ||||||||
| # API-returned headers applied as given, but Content-Length stays ours — explicit so S3 presigned URLs see no chunked encoding. | ||||||||
| response = client.put( | ||||||||
| url, | ||||||||
| content=tar_file, | ||||||||
| headers={ | ||||||||
| **{ | ||||||||
| k: v | ||||||||
| for k, v in (headers or {}).items() | ||||||||
| if k.lower() != "content-length" | ||||||||
| }, | ||||||||
| "Content-Length": str(size), | ||||||||
| }, | ||||||||
| ) | ||||||||
| response.raise_for_status() | ||||||||
| finally: | ||||||||
| # Closing the spooled temp file is best-effort: a failure here | ||||||||
|
|
||||||||
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.
When the API returns this header with any casing other than exactly
Content-Length(for example,content-length), the object retains both keys because JavaScript property names are case-sensitive, while Fetch header names are not. Undici combines the values (such as1, 123), causing the streamed PUT to fail with a content-length mismatch; the added test only covers the exact-case spelling. Delete or replace API-providedContent-Lengthcase-insensitively before setting the archive size.Useful? React with 👍 / 👎.
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.
Fixed in bfc487d — API-sent
Content-Lengthis now stripped case-insensitively before ours is set, in JS and both Python variants (same hole); the forcing tests now pass lowercasecontent-lengthto pin the behavior.