Repository navigation
feat(common): add presigned artifact byte delivery (3/6) - #14980
biswapanda wants to merge 1 commit into
Conversation
Signed-off-by: Biswa Panda <biswa.panda@gmail.com>
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
| url = self._strip_protocol(path) | ||
| headers = dict(kwargs.pop("headers", {})) | ||
| headers["Content-Length"] = str(len(value)) | ||
| session = await self.set_session() |
There was a problem hiding this comment.
Passing the presigned URL as a string makes aiohttp/yarl requote it before sending; for example, query %2F becomes / and %7E becomes ~. encoded=True is only an fsspec setting here and does not affect this direct session.put call, so otherwise valid signatures over the original URL bytes can be rejected.
🤖 AI Fix
Pass a yarl.URL constructed with encoded=True to session.put so the validated presigned URL is transmitted without requoting.
There was a problem hiding this comment.
[P3] The premise holds for the query string, but the stated consequence did not reproduce. aiohttp does rewrite %2F to / and %7E to ~ in the query. A real MinIO SigV4 presigned PUT still succeeded through this code path.
Measured: what changes on the wire, and what a real object store does with it
aiohttp 3.14.3, yarl 1.25.1. Local aiohttp server records request.raw_path.
| sent | received |
|---|---|
/o?X-Amz-Signature=a%2Fb&x=%7E&y=%2B&z=a+b&w=%20&v=%3D |
/o?X-Amz-Signature=a/b&x=~&y=%2B&z=a+b&w=%20&v=%3D |
/p%2Fq?sig=abc%2Fdef%3D%3D |
/p%2Fq?sig=abc/def%3D%3D |
/o?X-Amz-Credential=EXAMPLE%2F20260917%2Fus-west-2%2Fs3%2Faws4_request |
/o?X-Amz-Credential=EXAMPLE/20260917/us-west-2/s3/aws4_request |
The path is preserved. Only the query is requoted, and only %2F and %7E move.
End-to-end control against MinIO (quay.io/minio/minio:latest, boto3 s3v4, key a/b c~d.bin, presigned query contains %2F):
PUT via put_artifact -> OK, 19 bytes
get_object -> 19 b'exact-bytes-payload'
control raw PUT, same untouched URL -> HTTP 412
SigV4 re-encodes the decoded query when it rebuilds the canonical request, so %2F to / is a round trip for AWS-style signing. The 412 on the control is If-None-Match: * doing its job on the second write.
I did not test Azure SAS or Google signed URLs, so the risk for those providers is unverified. Passing yarl.URL(url, encoded=True) is still the safer call, and a query with a percent escape in the real-HTTP test would pin it. test_real_http_put_preserves_exact_target_and_body:375 uses a query with no percent escape, so it cannot catch a change here.
| url = self._strip_protocol(path) | ||
| headers = dict(kwargs.pop("headers", {})) | ||
| headers["Content-Length"] = str(len(value)) | ||
| session = await self.set_session() |
There was a problem hiding this comment.
This override bypasses HTTPFileSystem.encode_url() even though the filesystem is constructed with encoded=True. aiohttp canonicalizes a string URL, decoding/requoting percent-encoded query values; a valid presigned signature containing values such as %2F is therefore sent as a different URL and rejected by the object store. Preserve the raw signed target when issuing the PUT.
🤖 AI Fix
Pass self.encode_url(url) to session.put so the configured encoded=True preserves the exact presigned URL.
| raise ArtifactStorageError( | ||
| "presigned target lifetime exceeds operator limit" | ||
| ) | ||
| if normalized_headers.get("if-none-match") != "*": |
There was a problem hiding this comment.
The frontend protocol declares required_headers optional (#[serde(default)]), and target_from_settings likewise supplies {} when it is absent, but this new validation rejects every such otherwise-valid presigned_http_put target before any upload. This adds an adapter-only constraint that prevents presigned PUT providers which do not require conditional writes from being used.
🤖 AI Fix
Remove the unconditional If-None-Match: * requirement and forward the optional signed headers exactly as supplied.
| if expires <= datetime.now(timezone.utc): | ||
| raise ArtifactStorageError("presigned target has expired") | ||
| headers = dict(target.required_headers) | ||
| filesystem = _ExactHttpPutFileSystem( |
There was a problem hiding this comment.
fsspec caches filesystem instances by constructor token, so these identical options return the same _ExactHttpPutFileSystem for concurrent uploads. Each call then closes and clears the shared session in aclose, allowing one completed upload to abort another request that is still in flight.
🤖 AI Fix
Construct the filesystem with skip_instance_cache=True so every upload owns and closes an exclusive session.
There was a problem hiding this comment.
[P1] Confirmed by execution. Two overlapping put_artifact calls share one filesystem object, so the first one to finish closes the session the other is still using.
Measured A/B on the head tree, with the fix as the control
fsspec 2026.7.0, aiohttp 3.14.3, Python 3.12.9, macOS ARM64. Local aiohttp server: PUT /slow sleeps 1s, PUT /fast returns at once. The two uploads start 0.1s apart. _pipe_file is traced to count distinct filesystem objects.
| tree | distinct filesystem objects | slow upload | fast upload |
|---|---|---|---|
| head as-is | 1 | ArtifactStorageError: presigned artifact PUT failed |
OK |
head + skip_instance_cache=True |
2 | OK | OK |
The suppressed cause on the failing call is ClientConnectionError: Connector is closed.
Direct check of the cache:
_ExactHttpPutFileSystem(asynchronous=True, encoded=True, client_kwargs={...}) is <same call again>
-> True (cachable = True, cache size 1)
The one-line change I measured:
filesystem = _ExactHttpPutFileSystem(
asynchronous=True,
encoded=True,
+ skip_instance_cache=True,With that line the full new suite still passes (35 passed) and the A/B row above flips to two objects and two successes.
One test-coverage note that belongs with this. test_generation_artifact_storage.py:64 is named test_presigned_put_closes_each_request_filesystem and asserts two filesystems were made, but it patches _ExactHttpPutFileSystem with a factory, so it never reaches the fsspec cache and cannot see this. A test that awaits two overlapping put_artifact calls against a local server does reproduce it.
There was a problem hiding this comment.
[P1] Still live at the current head. Re-ran the same overlap at c844b95f: the two calls still share one filesystem object, and the slow upload fails. components/src/dynamo/common/generation_artifact_storage.py:317. Please add skip_instance_cache=True.
Re-measured at the current head, with a non-overlapping control
fsspec 2026.7.0, aiohttp 3.14.3, Python 3.12.9, macOS ARM64. Local aiohttp server: PUT /slow sleeps 1s, PUT /fast returns at once. The two uploads start 0.1s apart. _pipe_file is traced to count distinct filesystem objects.
| arm | distinct filesystem objects | slow upload | fast upload |
|---|---|---|---|
| head, overlapping | 1 | ArtifactStorageError: presigned artifact PUT failed |
OK |
| head, sequential (control) | 1 | OK | OK |
head + skip_instance_cache=True, overlapping |
2 | OK | OK |
The sequential control passes on the unchanged code, so the failure comes from the overlap and not from the probe.
CACHE identity f1 is f2 = True; cachable=True; cache_size=1
The file is byte-identical to the one I measured before: f20ff151338e997cf2e3ad1f84584f2290d3742dc426d9f7d95f31b9bbf6c905. With skip_instance_cache=True the whole new suite still passes (35 passed).
For the record on this round: the base branch tip is already the merge base, so merging it into this head is a no-op ("Already up to date") and changes none of the numbers above.
| @pytest.mark.parametrize( | ||
| "url", | ||
| [ | ||
| "https://127.0.0.1/object", |
There was a problem hiding this comment.
These IP-address rows do not distinguish the non-global-address guard they appear to cover: the fixture only allowlists storage.example and example.test, so both inputs still fail through the same unapproved-host outcome retained by the not-allowed.example row even if the IP-specific rejection is removed. The supported unapproved-host behavior remains protected by the hostname case; only keep an IP case if it allowlists that exact authority before asserting rejection.
🤖 AI Fix
Replace these two rows with one allowlisted non-global IP case, or remove them if this test is only meant to cover unapproved destinations.
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Review: presigned artifact byte delivery (3/6)
Not approving this round. One P1 is outstanding: concurrent uploads share a cached fsspec filesystem and close each other's session. I reproduced it with a control and replied on the existing bot thread rather than opening a new one.
What I verified, and where verification stopped
Stack position. Parts 1 to 6 are all open, so this cannot merge alone. Its base branch is 2/6 (#14979), which is based on 1/6 (#14977), which is based on main. Both of those must land first. Merge base with main is 3dd603b, dated today, and the branch carries one commit of its own. Nothing here is stale.
What the URL grants. This layer does not sign anything. It receives a presigned URL and delivers bytes to it. The bound on lifetime is the caller-declared expires_at, checked twice: at construction and again immediately before the PUT. The ceiling is DYN_GENERATION_ARTIFACT_MAX_PRESIGNED_TTL_SECONDS, default 3600 seconds. Measured on every malformed value: absent gives 3600; "", abc, 3600.0, 0x10, 1e5 are all rejected as invalid; 0 and -1 reject every target; a very large value is accepted, which is an operator choice rather than a caller one. A naive (no timezone) expires_at is rejected. The real signature lifetime inside the URL is not parsed, so the TTL rule bounds the declaration, not the credential.
Destination control. The path and query are entirely caller-supplied, by design, because the URL is the grant. The bound is the host allowlist, DYN_GENERATION_ARTIFACT_PRESIGNED_HOSTS, which is empty by default and therefore closed by default. I ran the hostile set. Rejected: a non-allowlisted host, an embedded credential (https://allowed@evil.test/x), a non-default port not named in the allowlist, a trailing-dot host, loopback, link-local, private and IPv6 loopback addresses, a fragment, an invalid percent escape, a CR/LF in the URL, and a non-HTTP scheme. Accepted inside an allowlisted host: .., a leading double slash, percent-encoded traversal, a Windows separator, and an empty path. Those are path bytes inside one already-signed grant, so nothing reaches a host the operator did not name.
Leak audit. The module writes no log line. repr, str and f-string formatting all redact the URL, the headers and the expiry. On a real connection failure to an allowlisted host, the caller receives ArtifactStorageError("presigned artifact PUT failed") with __cause__ and __context__ both None, and neither the host nor a signature value appears in the traceback or in captured logs at DEBUG. That matches the suppression 4/6 uses.
Boundedness. len(data) > max_bytes is checked before any socket is opened, Content-Length is pinned to the exact length, redirects are refused, and this is a single PUT rather than a multipart upload, so a truncated body cannot become a visible object. If-None-Match: * is required, which I saw return HTTP 412 on a second write to the same MinIO key.
Test reality. All 35 tests pass locally. Ten mutations of the guards turned nine of them red; the tenth is the P3 below. Marker selection measured rather than read: the file carries unit, gpu_0 and pre_merge, and pre_merge and not parallel and not defaulted and not (vllm or sglang or trtllm) and (gpu_0) from .github/workflows/dynamo-pipeline.yml:242 collects all 35. The three framework CPU lanes collect none, which is correct for this file.
Where verification stopped. I tested against MinIO only. Azure SAS and Google signed URL behavior under the aiohttp query requoting is unverified. I did not build a container image, so the fsspec downgrade below is reproduced with uv in the same install order rather than in the real image.
| --no-binary imageio-ffmpeg | ||
|
|
||
| aiohttp>=3.14.3,<4.0 | ||
| fsspec>=2026.4.0,<2027.0 |
There was a problem hiding this comment.
[P2] The new fsspec floor is not the version CI runs. container/deps/requirements.common.txt:15 sets fsspec>=2026.4.0, but container/Dockerfile.test then installs requirements.test.txt, whose datasets==4.4.1 caps fsspec<=2025.10.0 and downgrades it. Please lower the floor to what the code needs, or raise the datasets pin.
Measured downgrade, and the suite on the downgraded version
datasets==4.4.1 metadata on PyPI: fsspec[http]<=2025.10.0,>=2023.1.0.
Reproduced in the same order the images use. The runtime image installs requirements.common.txt, then container/Dockerfile.test:20-33 installs requirements.test.txt on top.
uv pip install 'fsspec>=2026.4.0,<2027.0' -> fsspec 2026.7.0
uv pip install 'datasets==4.4.1' -> - fsspec==2026.7.0
+ fsspec==2025.10.0
The new suite passes on 2025.10.0 (35 passed), so nothing is broken today. The effect is that the shipped ai-dynamo wheel declares a requirement the test image does not satisfy, and the floor is never exercised. Nothing in generation_artifact_storage.py needs 2026.4.0: HTTPFileSystem, set_session, and the encoded option all predate it.
There was a problem hiding this comment.
[P2] Stronger evidence: the two files cannot both be satisfied. A single solver over both requirement sets is unsatisfiable, so the sequential install silently wins and CI gets fsspec 2025.10.0. container/deps/requirements.common.txt:15.
Joint resolve fails outright; the image order downgrades instead
datasets==4.4.1 declares fsspec[http]<=2025.10.0,>=2023.1.0.
Put both constraints in one file and resolve:
fsspec>=2026.4.0,<2027.0
datasets==4.4.1
And because you require fsspec>=2026.4.0 and datasets==4.4.1, we can
conclude that your requirements are unsatisfiable.
The images do not resolve them together. They install requirements.common.txt first, then requirements.test.txt on top, so the second step downgrades in silence:
step 1: + fsspec==2026.7.0 -> after step 1: fsspec 2026.7.0
step 2: - fsspec==2026.7.0
+ fsspec==2025.10.0 -> after step 2: fsspec 2025.10.0
Control: the floor on its own still resolves to fsspec==2026.7.0.
So the declared floor is never the version under test. Nothing is broken today, because the new suite passes on 2025.10.0 (35 passed), and HTTPFileSystem, set_session and the encoded option all predate 2026.4.0. Please lower the floor to what the code needs, or raise the datasets pin.
| "url": "https://example.test/object", | ||
| "max_bytes": 1024, | ||
| "expires_at": (datetime.now(timezone.utc) + timedelta(minutes=5)).isoformat(), | ||
| "required_headers": {"if-none-match": "*"}, |
There was a problem hiding this comment.
[P3] The If-None-Match: * rule is the one guard no test pins. Replacing the check at generation_artifact_storage.py:177 with if False: leaves all 35 tests green. The PR body calls this the control that stops a retry overwriting an object, so please add a case that omits the header and expects a rejection.
Mutation run: nine of ten guards are pinned, this one is not
Each mutation was applied to a copy of the head tree on its own, then the whole new file was run. fsspec 2026.7.0, aiohttp 3.14.3, Python 3.12.9.
| mutation | result |
|---|---|
| drop the HTTPS scheme guard | red |
| drop the host allowlist | red |
allow_redirects=True |
red |
leak the provider cause instead of from None |
red |
drop the max_bytes pre-check |
red |
| drop the TTL ceiling | red |
| drop the denied-header set | red |
drop the URL redaction in __repr__ |
red |
| drop the expiry recheck before the network call | red |
drop the If-None-Match: * requirement |
green, 35 passed |
Control: the unmutated tree is 35 passed.
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: Concurrent uploads still construct a cacheable
_ExactHttpPutFileSystematgeneration_artifact_storage.py:317; fsspec therefore shares the instance and one call can close another in-flight call's session. The demonstratedskip_instance_cache=Truefix is still absent. - Original discussion: Still present: identical fsspec constructor arguments reuse one cached filesystem and session, while every put closes that session. Overlapping uploads can therefore cause the completing upload to abort another upload still in flight.
- Original discussion: Still present: the new common dependency requires fsspec >=2026.4.0, while the test image layers datasets==4.4.1 on top of the runtime image; its fsspec upper bound downgrades the installed version, so CI does not satisfy or exercise the declared floor.
- Original discussion: The IP-address rows still do not exercise the non-global-IP rejection because the autouse fixture does not allowlist either IP authority; both cases fail the allowlist check instead.
- Original discussion: The new
fsspec>=2026.4.0floor remains incompatible with the test image's laterdatasets==4.4.1installation, which downgrades fsspec to its<=2025.10.0cap. CI therefore does not exercise the declared dependency floor. - Original discussion: The required
If-None-Match: *guard remains untested: the current suite has no valid target missing that header and expecting rejection, so removing the guard still leaves the suite green. - Original discussion: The two IP-address rows in
test_presigned_put_rejects_unapproved_or_ambiguous_destinationsare still non-discriminating: with the fixture allowlist limited tostorage.example,example.test, removing the non-global-address guard leaves both IP cases failing through the same unallowlisted-host path already covered byhttps://not-allowed.example/object. - Original discussion: Verified still present: identical
_ExactHttpPutFileSystemconstructions use fsspec's instance cache, while everyput_artifactcall closes and clears that shared session. Concurrent uploads can therefore fail when one completed upload closes the connector used by another in-flight upload. - Original discussion: Verified still present: identical fsspec constructor arguments reuse one cached filesystem, while each put closes its session. Overlapping uploads can therefore close another in-flight upload's connector and fail it.
- Original discussion: Verified still present: the Rust protocol makes required_headers optional, but this adapter rejects an otherwise valid presigned_http_put target unless it contains If-None-Match: *. This narrows the declared protocol and blocks providers without conditional-write headers.
- Original discussion: Verified still present: the new
fsspec>=2026.4.0runtime requirement conflicts withdatasets==4.4.1in the test dependency layer, which caps fsspec at 2025.10.0. The test image can downgrade fsspec below the package's declared floor. - Original discussion: Verified still present: the new fsspec>=2026.4.0 floor conflicts with the test image's datasets==4.4.1 dependency cap, so the test-image installation downgrades fsspec below the declared requirement.
| return any(ord(character) < 32 or ord(character) == 127 for character in value) | ||
|
|
||
|
|
||
| def _presigned_host_allowed(parsed) -> bool: |
There was a problem hiding this comment.
The allowlist check rejects non-global literal IPs, but hostnames are accepted without resolving them. The later aiohttp PUT resolves an allowlisted hostname independently, so a split-horizon or rebinding DNS answer such as 127.0.0.1 or a metadata-service address receives the artifact bytes despite the documented private-address restriction.
🤖 AI Fix
Use an aiohttp resolver or connector that validates every resolved address is global before connecting while retaining the original hostname for TLS and Host handling.
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Follow-up round, re-verified at c844b95f
Not approved. One P1 is still live, so the bar of zero P1 is not met. The head has not moved since the last round, and the base branch tip is already the merge base, so nothing changed under this diff.
Outstanding: 1 P1, 1 P2, 2 P3.
What I re-ran, and what each of the four findings now shows
| finding | file:line | outcome this round |
|---|---|---|
| P1 shared filesystem object | generation_artifact_storage.py:317 |
still live; overlap fails, sequential control passes |
| P2 dependency floor | requirements.common.txt:15 |
still live; a joint resolve is now shown to be unsatisfiable |
| P3 URL requoting | generation_artifact_storage.py:70 |
unchanged; the consequence still does not reproduce, so no new ask |
| P3 conditional-write guard untested | test_generation_artifact_storage.py:292 |
still unpinned; the mutation stays green |
The mutation for the last row replaces the check at generation_artifact_storage.py:177 with if False:. All 35 tests still pass. Two control mutations confirm the harness does detect changes: removing the HTTPS scheme guard gives 2 failed, 33 passed, and removing the host allowlist gives 4 failed, 31 passed. Every mutated file had a checksum different from the baseline, and every restored file matched it again.
Coverage is good: a lane does run all 35 new tests
This is the positive half, and it is worth recording because the file carries no backend marker.
The new file is marked unit, gpu_0, pre_merge. Every backend lane deselects it:
-m 'pre_merge and vllm and gpu_0 and not e2e' -> no tests collected (35 deselected)
-m 'pre_merge and sglang and gpu_0' -> no tests collected (35 deselected)
-m 'pre_merge and trtllm and gpu_0' -> no tests collected (35 deselected)
-m 'pre_merge and triton and gpu_0' -> no tests collected (35 deselected)
-m 'pre_merge and planner and gpu_0' -> no tests collected (35 deselected)
The catch-all lane picks it up instead:
-m 'pre_merge and not parallel and not defaulted and not (vllm or sglang or trtllm) and (gpu_0)'
-> 35 tests collected
That expression belongs to the sequential job in .github/workflows/dynamo-pipeline.yml:242. Its log for this head lists all 35 tests as PASSED, with no skips. So the new code is covered, and no marker change is needed.
This branch has not been deployed
2 participants
Purpose
Provide a generic exact-byte delivery primitive for presigned object-store uploads. This layer receives opaque bytes and an authorized destination; it does not know whether those bytes contain routed experts, logprobs, another backend format, or a future artifact type.
What changes
PresignedHttpPutTarget,ArtifactReceipt, target parsing, validation, and asynchronous exact-byte PUT delivery underdynamo.common.Content-Length, redirects disabled, bounded connect/total timeouts, and deterministic session cleanup.If-None-Match: *so a retry cannot silently overwrite an existing object.fsspecdependency to common Dynamo packaging.Security model
DYN_GENERATION_ARTIFACT_PRESIGNED_HOSTS.DYN_GENERATION_ARTIFACT_ALLOW_INSECURE_HTTPand an exact authority inDYN_GENERATION_ARTIFACT_INSECURE_HTTP_HOSTSare configured.expires_atmust be an unexpired RFC 3339 timestamp within the operator TTL limit, which defaults to 3600 seconds and is controlled byDYN_GENERATION_ARTIFACT_MAX_PRESIGNED_TTL_SECONDS.Failure and recovery semantics
This is a single exact PUT. It does not retain grants for another process, resume an interrupted upload, or issue a provider status check after an ambiguous commit where storage accepted the bytes but the response was lost. In-process cancellation handling is added at the capture pipeline in #14975; cross-worker recovery would require a separate durable grant/reconciliation design.
Reviewer guide
components/src/dynamo/common/generation_artifact_storage.py: target validation and presigned PUT implementation.components/src/dynamo/common/tests/test_generation_artifact_storage.py: allowlist, header, TTL, overwrite, timeout, cleanup, and local HTTP coverage.Validation
PR stack
Review and merge bottom-up. Each PR is based on the preceding stack branch, so its diff contains only that layer. The original umbrella #14385 remains open and unchanged as a reference.