Skip to content

fix: preserve result body in async FAILURE responses#178

Merged
dm36 merged 1 commit intomasterfrom
dhruv/update-responses
Mar 26, 2026
Merged

fix: preserve result body in async FAILURE responses#178
dm36 merged 1 commit intomasterfrom
dhruv/update-responses

Conversation

@dm36
Copy link
Collaborator

@dm36 dm36 commented Mar 26, 2026

Summary

  • The server (llm-engine) now returns result as a plain string (stringified exception message) on FAILURE, not a nested dict with result/result_url keys
  • Updated FAILURE branch to read result directly from async_response instead of trying to unpack it as a dict
  • result_url is always None on failure — no model output is produced when a task fails
  • Companion to llm-engine PR #787 which surfaces str(res.result) and res.traceback in GetAsyncTaskV1Response for FAILURE states

Test Plan

  • Updated test_endpoint_response_future_failure_preserves_result to reflect the new response shape: result is a plain string, traceback is the full stack trace
  • Existing test_endpoint_response_future_failure_no_result_body still covers the None result case (e.g. hard pod kill)

Greptile Summary

This PR fixes the FAILURE branch in EndpointResponseFuture.get() to correctly preserve the result field from async task responses. Previously, on FAILURE, the result was hardcoded to None, discarding the server's stringified exception message. Now, result is read directly from async_response (via .get("result", None)), aligning with the updated llm-engine server behavior where result is a plain string on failure rather than a nested dict.

  • launch/model_endpoint.py: Changed result=None to result=async_response.get("result", None) in the FAILURE branch, preserving the error message from the server.
  • tests/test_model_endpoint.py: Added two new tests — one verifying that result and traceback are correctly populated on failure, and another covering the None result case (e.g., pod crash with no response body).

Confidence Score: 5/5

This PR is safe to merge — it is a minimal, well-tested bug fix with no risk of regression.

The change is a single-line fix that replaces a hardcoded None with a safe .get() call. The behavior for SUCCESS is untouched. Two new tests directly cover the changed code path, including the edge case. No breaking changes to the public API.

No files require special attention.

Important Files Changed

Filename Overview
launch/model_endpoint.py Single-line change to read result from async_response instead of hardcoding None in the FAILURE branch. Correct and minimal.
tests/test_model_endpoint.py Added two well-structured tests covering both the new result-preserving behavior and the existing None result edge case on failure.

Sequence Diagram

sequenceDiagram
    participant User
    participant EndpointResponseFuture as EndpointResponseFuture.get()
    participant Client as LaunchClient
    participant Server as llm-engine

    User->>EndpointResponseFuture: get()
    loop Poll until terminal status
        EndpointResponseFuture->>Client: _get_async_endpoint_response()
        Client->>Server: GET /async-task/{id}
        Server-->>Client: {status, result, traceback, status_code}
        Client-->>EndpointResponseFuture: async_response dict
    end
    alt status == SUCCESS
        EndpointResponseFuture-->>User: EndpointResponse(result=response.result.result, result_url=response.result.result_url)
    else status == FAILURE
        Note right of EndpointResponseFuture: Before: result=None (discarded)<br/>After: result=async_response["result"] (preserved)
        EndpointResponseFuture-->>User: EndpointResponse(result=error_string, traceback=traceback, result_url=None)
    end
Loading

Reviews (2): Last reviewed commit: "fix: preserve result body in async FAILU..." | Re-trigger Greptile

@dm36 dm36 requested a review from lilyz-ai March 26, 2026 17:29
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@dm36 dm36 force-pushed the dhruv/update-responses branch from d638fbb to e5a0824 Compare March 26, 2026 19:42
@dm36 dm36 merged commit d6a1d6a into master Mar 26, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants