Skip to content

python/etl: classify direct-put responses by Ais-Direct-Put-Complete marker#334

Draft
chanu1406 wants to merge 1 commit into
NVIDIA:mainfrom
chanu1406:python-direct-put-content-length-parity
Draft

python/etl: classify direct-put responses by Ais-Direct-Put-Complete marker#334
chanu1406 wants to merge 1 commit into
NVIDIA:mainfrom
chanu1406:python-direct-put-content-length-parity

Conversation

@chanu1406

@chanu1406 chanu1406 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Follow up to #330

Reworking per the review discussion. classify direct-put responses by the new Ais-Direct-Put-Complete marker instead of the read body.

  • The AIS target returns 204 + Ais-Direct-Put-Complete + Ais-Direct-Put-Length after a successful direct put. The Python webservers now classify by marker, a marked response is the delivered ack, passed back through the pipeline as is: the 204, the marker, and the length (even when the length is 0 for an empty object).
  • Markerless responses fall through to the existing legacy handling unchanged, kept as a fallback for targets that predate the marker, to be phased out with them.
  • This PR only works on the Python webservers. the target-side 204 change and Go webserver handling will be in a separate PR

@chanu1406
chanu1406 requested a review from alex-aizman as a code owner July 9, 2026 01:09

@gaikwadabhishek gaikwadabhishek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs some more work

return resp.status_code, resp.content, 0

return STATUS_NO_CONTENT, b"", size # from target, no content
# Keyed on the Content-Length header, mirroring the Go webserver's

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the original issue for a 2-stage same-target pipeline, but not for 3+ stages.

The final streaming ETL can return 200 with an empty chunked body and no Content-Length, meaning the transformed object is valid but empty. The previous ETL now recognizes that correctly. However, when it forwards the result, the Python server turns it into a buffered empty response with Content-Length: 0. If there is another ETL upstream, that ETL sees Content-Length: 0, assumes the object was already direct-put to the target, and returns 204. In the same-target case no direct put actually happened, so AIS skips writing the empty object.

With only 2 stages, the rewritten 200 goes directly back to AIS, which accepts it as an empty object, so that case works. This is a partial fix to an existing bug, not a new regression. We still need to preserve the unknown/chunked-length state when forwarding the empty response and add a 3-stage test.

This specifically affects same-target pipelines, where the source and destination resolve to the same AIS target. In that case no final direct PUT occurs, so the incorrect 204 causes AIS to skip the actual write.

can you please debug this @chanu1406, its slightly more complex that it seems. We are mixing streaming + pipeline + direct put which is creating a complex situation. If possible add a test but its difficult because how would you know before hand whether on which target the object will be stored. Lmk if you need help

@chanu1406

Copy link
Copy Markdown
Contributor Author

@gaikwadabhishek Thanks for the review, this is a valuable finding

can you let me know what you think of this approach for the python servers:

The issue is the middle stage classifies the empty chunked 200 correctly, but then re-emits it as a buffered empty response. All three Python servers stamp Content-Length: 0 on an empty 200 body, which makes it indistinguishable from the target's direct-put ack to the next stage up.

Approach: for transform-content responses, the Python servers should never emit 200 + Content-Length: 0. If the transformed body is empty, emit it with unknown-length framing instead:

  • FastAPI: StreamingResponse(iter(()))
  • Flask: iterable response body
  • HTTP server: omit Content-Length, matching the existing streaming path

This keeps 200 + Content-Length: 0 reserved for the target/direct-put ack. Non-empty responses, errors, real 204 direct-put responses, health checks, and websocket behavior stay unchanged. Applying the same rule to the no-pipeline response also fixes another case: a buffered final stage returning an empty result emits the same 200 + Content-Length: 0, so the upstream stage misreads it as delivered even with just two stages.

For coverage, test_etl_webserver_pipeline.py already chains real webservers over localhost with the test client acting as the AIS-facing caller. A 3-stage pipeline with no final target URL should match the same-target topology, so we can add a regression test where the final stage returns an empty result and assert the caller gets 200 with an empty body instead of 204.

@gaikwadabhishek

Copy link
Copy Markdown
Member

@Nahemah1022 thoughts?

@chanu1406

Copy link
Copy Markdown
Contributor Author

@gaikwadabhishek @Nahemah1022 I looked into this issue a bit further, and had some findings.

The problem with my earlier idea is that whether a response is chunked or has Content-Length: 0 is just how the body got framed on that one hop, so any server in between is free to buffer it and stamp Content-Length: 0 back on. So framing can't reliably tell us who sent the response. I also found that the three *_to_target_pipeline_chain tests and test_websocket_to_target_direct_pipeline test actually fail on this branch, because their mocked target ack has no Content-Length and the CL based check misreads it as content, so the object length is lost (the tests fail on the delivery details, not the status).

However the stage already knows what it just sent the PUT to. ETL pipeline entries are host-only URLs, while the destination target URL carries the object path — compose_etl_direct_put_url and Go's directPut already route on exactly that distinction. So we should classify the response by the hop, not the header:

  • 200 from an ETL stage → it's content, always — even an empty body with Content-Length: 0
  • 200 from the destination target → direct put succeeded → convert to 204 (the target never returns a body on a successful PUT)
  • 204 → already delivered, pass it through

Content-Length stops mattering in deciding whether delivery occurred, which matches the documented contract in core/ldp.go (200 = content, 204 = delivered). And it needs no changes at all to how the servers build their responses, so none of the StreamingResponse/framing stuff from my last approach.

Still the same 3 stage regression test plan as before. The failing to_target tests pass again without touching their mocks, and I'll switch the ETL hop URLs in the existing tests to host-only so they match what AIS actually sends. The Go webserver has the same issue on its side, can follow up on that as well. Let me know what you think

@Nahemah1022

Copy link
Copy Markdown
Collaborator

I think the core problem is the ambiguity around whether a 200 response comes from the destination target or another ETL stage.

@chanu1406 You’re right that inspecting the URL could help distinguish between them, but I’m concerned that this would introduce another branch of logic to maintain on top of the already complicated pipeline logic.

I wonder whether it would make our lives easier if we updated the AIS target to return 204 after a successful direct PUT and simply propagated that response all the way back? Fortunately, the dedicated ETL direct-PUT endpoint (/v1/etl/_object/...) is only used by ETL pods, so changing its status-code contract should have limited impact.

This seems like it could remove the ambiguity entirely: ETL responses would always use 200, including for empty output, while 204 would unambiguously mean that the object has already been stored. Each ETL stage would only need to propagate the 204 status and Ais-Direct-Put-Length header back to the originating target.

This is just an idea. @chanu1406 Could you confirm whether this approach would work better based on your understanding of the core issue?

@chanu1406

Copy link
Copy Markdown
Contributor Author

@Nahemah1022 After looking into it, I agree this is a cleaner approach and it should work well.

The target currently returns the 200 implicitly after a successful /v1/etl/_object/... PUT, and both the Python and Go webservers already handle and propagate 204 so this fits in nicely.

The target would return 204 with Ais-Direct-Put-Length (it knows exactly what it wrote), and I'd simplify the webservers so 200 always means ETL output, including an empty body. removes the need to inspect URL or Content-Length

However, while tracing, I found that there may be a compatibility issue. A webserver using the new semantics but talking to an older target would treat the target’s legacy bare 200 acknowledgment as empty ETL output. In a different-target pipeline, AIS would then send that empty result to the destination and overwrite the object that was just direct-put.

To avoid that, either all targets must be upgraded before deploying webservers with the new behavior, or we keep a
temporary fallback only when the next hop is known to be the destination target, then remove it after the compatibility window. a general Content-Length fallback would bring back the same ambiguity we’re removing

@gaikwadabhishek gaikwadabhishek added python Pull requests that update Python code etl labels Jul 11, 2026
@Nahemah1022

Copy link
Copy Markdown
Collaborator

@chanu1406 I think we can still keep the backward-compatibility handling very small without introducing URL-based logic.

Can we add a behavior-based response header whose presence indicates that a 200 response is ETL output and must be treated as such, even when its body is empty?

The shared response handler in both Go and Python could then have a clean if-else branch:

  • If the header is present, treat the response as ETL output and forward it unchanged, including an empty body.
  • Otherwise, fall through to the existing response-handling logic unchanged.

Every updated ETL webserver should add this marker to successful 200 output responses. Meanwhile, after a successful direct PUT, the AIS target would return 204 with Ais-Direct-Put-Length, and each ETL stage would propagate that status and length back through the pipeline.

This provides a clean compatibility path:

  • An old target’s bare 200 has no marker, so it follows the existing legacy handling.
  • A new ETL server’s empty 200 has the marker, so it is correctly preserved as empty ETL output.
  • A new target returns 204 with Ais-Direct-Put-Length, which is propagated directly.

Once the new behavior has been deployed and stabilized, we can remove the legacy fallback. Would this fit cleanly into the current Go and Python implementations?

@chanu1406

Copy link
Copy Markdown
Contributor Author

@Nahemah1022 Sounds good, it should fit cleanly into both implementations.

  • For Python, every direct-put response already funnels through the shared handle_direct_put_response so the if else would be in one place, with current logic as the legacy fallback. each server would add the marker where it builds its 200 output responses (header only change)
  • For Go, it's similar with directPut, andsetResponseHeaders is already the shared helper for output responses, so the marker would be there.
  • On the target side, putObjectETL returns the 200 implicitly today, so switching to 204 + Ais-Direct-Put-Length is a small change. the marker could be something like Ais-Etl-Output next to the existing constants in api/apc/headers.go, mirrored in the Python SDK constants.
  • This also covers the issue from my last comment, an old target's bare 200 has no marker, so it falls into the fallback, which already treats Content-Length: 0 as delivered, there would be no overwrite. Old webservers ignore the unknown header, so mixed versions don't regress; the empty output fix works once both stages of a hop run the new SDK.

I can work on the PR with this approach. Do you prefer the target side 204 change and the Go webserver work to go in this PR as well, or separate it?

@Nahemah1022

Copy link
Copy Markdown
Collaborator

@chanu1406 Thanks, that sounds good. For the header name, I prefer Ais-Direct-Put-Complete. It describes a fact that the target successfully stored the object through the direct-PUT endpoint.

The target should set it alongside 204 and Ais-Direct-Put-Length. Intermediate ETL webservers should propagate it.

The compatibility handling can then remain very small:

if Ais-Direct-Put-Complete is present:
    the destination already stored the object
    propagate 204 + Ais-Direct-Put-Complete + Ais-Direct-Put-Length
else:
    use the existing legacy handling unchanged

This keeps the compatibility path isolated. Around the next major release or so, we can deprecate and gradually phase out the legacy handling.

I also think the Go and Python changes should be split into separate PRs, with their own focused testing. The target-side 204 change and Go webserver handling can be one PR, followed by a separate Python webserver PR.

@chanu1406

Copy link
Copy Markdown
Contributor Author

@Nahemah1022 Sounds good, I'll start working on it

Also, the case with the empty transform result may not be fully fixed yet with this. When a middle stage buffers it, it comes out as 200 + Content-Length: 0, so the legacy handling will read it as delivered. This can only get resolved once the legacy fallback is removed, so I'll leave out the empty-output regression test that I mentioned earlier.

@chanu1406 chanu1406 changed the title python/etl: classify direct-put 200 responses by Content-Length python/etl: classify direct-put responses by Ais-Direct-Put-Complete marker Jul 14, 2026
@chanu1406
chanu1406 force-pushed the python-direct-put-content-length-parity branch from c2d0a91 to 21926ed Compare July 14, 2026 21:59
@gaikwadabhishek

Copy link
Copy Markdown
Member

Add a brief desc of the changes and important things in this PR (in bullets). Check other commits for suggestions

…marker

* the AIS target acks a successful direct PUT with 204 + Ais-Direct-Put-Complete
  + Ais-Direct-Put-Length; webservers classify by marker presence and propagate
  the ack back through the pipeline - status, marker, and length (including 0
  for an empty stored object)
* markerless responses fall through to the existing Content-Length-keyed
  handling, kept as a legacy fallback for targets that predate the marker
* handle_direct_put_response now returns (status, body, length, complete);
  all three servers (FastAPI, Flask, HTTP) thread the flag through the
  buffered, streaming, and websocket paths
* tests: marker classification, multi-stage ack propagation (incl. zero-length),
  legacy-target fallback; target-ack mocks updated to the new contract
* Python webservers only; target-side 204 + Go webserver handling in a follow-up PR

Signed-off-by: Chanu Ollala <chanuollala@gmail.com>
@chanu1406
chanu1406 force-pushed the python-direct-put-content-length-parity branch from 21926ed to d2a3aad Compare July 15, 2026 18:31
@chanu1406

Copy link
Copy Markdown
Contributor Author

Add a brief desc of the changes and important things in this PR (in bullets). Check other commits for suggestions

@gaikwadabhishek amended the commit

@Nahemah1022

Copy link
Copy Markdown
Collaborator

@chanu1406 Thanks for updating this. Can we treat this PR as blocked on the target/Go changes and land those first?

Without the target returning 204 with Ais-Direct-Put-Complete and Ais-Direct-Put-Length, the new Python path cannot be exercised against a real AIS deployment—the current tests can only mock the expected response.
I suggest opening and landing the target-side and Go webserver PR first, then rebasing this PR and testing the Python implementation against the updated target.

@NVIDIA NVIDIA deleted a comment from majdalwsoof1232580-hub Jul 21, 2026
@chanu1406

Copy link
Copy Markdown
Contributor Author

@chanu1406 Thanks for updating this. Can we treat this PR as blocked on the target/Go changes and land those first?

Without the target returning 204 with Ais-Direct-Put-Complete and Ais-Direct-Put-Length, the new Python path cannot be exercised against a real AIS deployment—the current tests can only mock the expected response. I suggest opening and landing the target-side and Go webserver PR first, then rebasing this PR and testing the Python implementation against the updated target.

Sounds good, I will open a separate PR once I finish implementing

@gaikwadabhishek
gaikwadabhishek marked this pull request as draft July 23, 2026 22:58
@gaikwadabhishek

Copy link
Copy Markdown
Member

Moving this MR to draft as Python side changes look complete, but Go (server side) changes need to land first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

etl python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants