aws / aws/aws-lambda-rust-runtime

Fix RuntimeApiClientFuture Visibility and Response Handling

Open
#1,110 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
Rust
Stars
3.6k
Forks
396
PR merge metrics
No merged PRs in 30d

Description

## Background

The `RuntimeApiClientFuture` enum in `lambda-runtime/src/layers/api_client.rs` has several design issues that prevent proper error handling and violate Rust API design best practices. These issues were identified during discussions in PRs [#1091](https://github.com/aws/aws-lambda-rust-runtime/pull/1091) and [#1105](https://github.com/aws/aws-lambda-rust-runtime/pull/1105).

## Current Problems

### 1. Missing `#[non_exhaustive]` Attribute

The `RuntimeApiClientFuture` enum is publicly exposed but lacks the `#[non_exhaustive]` attribute:

```rust
pub enum RuntimeApiClientFuture {
First(#[pin] F, Arc),
Second(#[pin] BoxFuture<'static, Result, BoxError>>),
}
```

This is a semver hazard because:
- Adding new variants becomes a breaking change for downstream users who pattern match on this enum
- The enum is accessible as an associated type on `RuntimeApiClientService`, which is publicly visible
- Any user building custom runtimes on top of our runtime client could be affected

### 2. Incorrect Visibility

The enum and its variants are fully public, exposing internal implementation details that were likely not intended to be part of the public API. As noted in PR #1105:

### 3. Cannot Access Response Body

The current implementation discards the response body for non-2xx responses, making it impossible to read error details from the Lambda Runtime API:

```rust
Ok(resp) if !resp.status().is_success() => {
let status = resp.status();

log_or_print!(
tracing: tracing::error!(status = %status, "Lambda Runtime API returned non-200 response"),
fallback: eprintln!("Lambda Runtime API returned non-200 response: status={status}")
);

// We cannot access resp.body() here to log the actual error message!

break Ok(()); // Returns Ok despite error!
}
```

This is particularly problematic for 410 Gone responses (function timeouts), where we have to add hardcoded messages instead of reading the actual error from the API.

### 4. Returns `Ok(())` for Non-2xx Status Codes

The most counterintuitive behavior is that the client returns `Ok(())` even when the Lambda Runtime API returns error status codes:

```rust
Ok(resp) if !resp.status().is_success() => {
// ... logging ...

// Return Ok to maintain existing contract - runtime continues despite API errors
break Ok(());
}
```

This makes error handling confusing. A non-2xx HTTP response should be treated as an error, not success.

## Impact Assessment

Based on code search analysis mentioned in PR #1105:

The impact is expected to be minimal:
- Internal machinery doesn't rely on the specific type surface
- Only users who built custom higher-level runtimes would be affected
- Forks of the api client would need updates

## Proposed Solutions

Since these changes are breaking, we should bundle them together in a single major version bump. Here are the recommended approaches:

### Option 1: Add `#[non_exhaustive]` and Fix Response Handling (Recommended)

**Changes:**
1. Add `#[non_exhaustive]` to `RuntimeApiClientFuture`
2. Change the return type to properly represent errors
3. Read and log response bodies for non-2xx responses
4. Return `Err` for non-2xx status codes instead of `Ok`

**Implementation:**
```rust
#[non_exhaustive]
pub enum RuntimeApiClientFuture {
First(#[pin] F, Arc),
Second(#[pin] BoxFuture<'static, Result, BoxError>>),
}

impl Future for RuntimeApiClientFuture
where
F: Future, BoxError>>,
{
type Output = Result<(), BoxError>;

fn poll(mut self: Pin<&mut Self>, cx: &mut task::Context<'_>) -> task::Poll {
task::Poll::Ready(loop {
match self.as_mut().project() {
// ... First variant handling ...
RuntimeApiClientFutureProj::Second(fut) => match ready!(fut.poll(cx)) {
Ok(resp) if !resp.status().is_success() => {
let status = resp.status();

// Read the response body to get actual error details
let body = read_body_to_string(resp).await;

log_or_print!(
tracing: tracing::error!(
status = %status,
body = %body,
"Lambda Runtime API returned non-2xx response"
),
fallback: eprintln!(
"Lambda Runtime API returned non-2xx response: status={status}, body={body}"
)
);

// Return Err for non-2xx responses
break Err(format!("Runtime API error: {} - {}", status, body).into());
}
Ok(_) => break Ok(()),
Err(err) => break Err(err),
},
}
})
}
}
```

**Pros:**
- Fixes all issues in one go
- Proper error semantics
- Better debugging experience with actual error messages
- Future-proof with `#[non_exhaustive]`

**Cons:**
- Breaking change for anyone matching on the enum
- Breaking change for anyone relying on `Ok(())` for non-2xx responses

### Option 2: Deprecate and Replace with New Type

**Changes:**
### Option 2: Deprecate and Replace with New Type

**Changes:**
1. Create a new `RuntimeApiClientFutureV2` with correct attributes
2. Create a new `RuntimeApiClientServiceV2` that uses the new future type
3. Deprecate the old `RuntimeApiClientFuture` and `RuntimeApiClientService`
4. Update internal usage to new types
5. Remove old types in next major version

**Rationale:**
Since `RuntimeApiClientFuture` is exposed as an associated type on `RuntimeApiClientService`, we cannot change the future type without also creating a new service type. The service's `Future` associated type is part of its public API contract.

**Implementation:**
```rust
// Old types - deprecated
#[deprecated(since = "0.x.0", note = "Use RuntimeApiClientServiceV2 instead")]
pub struct RuntimeApiClientService {
inner: S,
client: Arc,
}

#[deprecated(since = "0.x.0", note = "Use RuntimeApiClientFutureV2 instead")]
pub enum RuntimeApiClientFuture {
First(#[pin] F, Arc),
Second(#[pin] BoxFuture<'static, Result, BoxError>>),
}

// New types - correct design
pub struct RuntimeApiClientServiceV2 {
inner: S,
client: Arc,
}

impl Service for RuntimeApiClientServiceV2
where
S: Service,
S::Future: Future, BoxError>>,
S::Error: Into,
{
type Response = ();
type Error = BoxError;
type Future = RuntimeApiClientFutureV2;

// ... implementation ...
}

#[non_exhaustive]
pub enum RuntimeApiClientFutureV2 {
First(#[pin] F, Arc),
Second(#[pin] BoxFuture<'static, Result, BoxError>>),
}
```

**Pros:**
- Gentler migration path
- Gives users time to update
- Old behavior remains available during deprecation period

**Cons:**
- Maintains broken behavior longer
- Significantly more code to maintain during transition (duplicate service + future implementations)
- Still requires breaking change eventually
- Users need to update both service and future type references
- More complex migration story

### Option 3 Changing the overall behaviour of client returning body.

Another solution proposed by @jlizen is to directly change how the client works at in `lambda-rust-api-client`. Like the following.

```rust
self.client
.request(req)
.map_err(Into::into)
.then(|res| async move {
match res {
Ok(resp) if !resp.status().is_success() => {
let status = resp.status();
match resp.into_body().await {
// convert into a BoxError containing the status and optional body
// which is nicely loggable in the upper layer, or you could also log it here.
// If you need to specifically log ONLY this case rather than other BoxErrors, you could newtype
// a marker type that we downcast the stderror to
Err(BoxError::new(format!("my message with status code + body")))
}
}
_ => res,
}
})
.boxed()
```

**Pros:**
- In this case we are only breaking the behaviour of client returning OK instead of ERR. It is a behavioural breaking chane.
- Proper error semantics
- Better debugging experience with actual error messages

**Cons:**
- `RuntimeApiClientFuture` is not fixed
- await at `Client` level.

### Error Types

Consider creating a dedicated error type for Runtime API errors:

```rust
#[derive(Debug, thiserror::Error)]
pub enum RuntimeApiError {
#[error("Runtime API returned {status}: {body}")]
NonSuccessResponse { status: StatusCode, body: String },

#[error("Request build failed: {0}")]
RequestBuildError(#[source] BoxError),

#[error("Network error: {0}")]
NetworkError(#[source] BoxError),
}
```

## References

- PR #1091: https://github.com/aws/aws-lambda-rust-runtime/pull/1091
- PR #1105: https://github.com/aws/aws-lambda-rust-runtime/pull/1105
- PR https://github.com/aws/aws-lambda-rust-runtime/pull/1109
- Current implementation: `lambda-runtime/src/layers/api_client.rs`

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.