Skip to content

python/etl: classify direct-put 200 responses by Content-Length#334

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

python/etl: classify direct-put 200 responses by Content-Length#334
chanu1406 wants to merge 1 commit into
NVIDIA:mainfrom
chanu1406:python-direct-put-content-length-parity

Conversation

@chanu1406

Copy link
Copy Markdown
Contributor

Follow up to #330

Fix Python ETL direct-put handling to match the Go webserver behavior from #330.

  • Python now uses the Content-Length header to tell whether a 200 OK direct-put response means “delivered to target” or “forward this transformed content.” This fixes the empty chunked response case, where a valid empty transform result was previously treated as already delivered.

I have also added unit coverage for the direct-put response cases, and updated the outdated comment in the Go webserver

Signed-off-by: Chanu Ollala <chanuollala@gmail.com>
@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
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