Skip to content

Treat connection upgrade (HTTP 101) as normal success.#1320

Open
lifning wants to merge 1 commit intomainfrom
switching-protocols-fix
Open

Treat connection upgrade (HTTP 101) as normal success.#1320
lifning wants to merge 1 commit intomainfrom
switching-protocols-fix

Conversation

@lifning
Copy link
Contributor

@lifning lifning commented Feb 27, 2026

This fixes a bug wherein client code would be generated for a 2XX-range response on WebSocket endpoints, which in turn would unnecessarily trip an assert!(response_types.len() <= 1) in extract_responses when explicit status codes are provided in the OpenAPI document (oxidecomputer/dropshot#1554). This also fixes a bug in such a scenario wherein all modeled response codes, including HTTP errors, would be set to OperationResponseKind::Upgrade.

Prior to this change, progenitor (incorrectly) outputs:

/// Sends a `GET` request to
/// `/v1/instances/{instance}/serial-console/stream`
pub async fn send(self) -> Result<ResponseValue<reqwest::Upgraded>, Error<reqwest::Upgraded>> {
    // [snip...]
    match response.status().as_u16() {
        101u16 => ResponseValue::upgrade(response).await,
        200..=299 => ResponseValue::upgrade(response).await,
        _ => Err(Error::UnexpectedResponse(response)),
    }
}

With this change alone, progenitor instead outputs:

/// Sends a `GET` request to
/// `/v1/instances/{instance}/serial-console/stream`
pub async fn send(self) -> Result<ResponseValue<reqwest::Upgraded>, Error<()>> {
    // [snip...]
    match response.status().as_u16() {
        101u16 => ResponseValue::upgrade(response).await,
        _ => Err(Error::UnexpectedResponse(response)),
    }
}

With this change plus the explicitly-modeled error codes ( oxidecomputer/dropshot#1554 ), progenitor outputs:

/// Sends a `GET` request to
/// `/v1/instances/{instance}/serial-console/stream`
pub async fn send(self) -> Result<ResponseValue<reqwest::Upgraded>, Error<()>> {
    // [snip...]
    match response.status().as_u16() {
        101u16 => ResponseValue::upgrade(response).await,
        400u16..=499u16 => Err(Error::ErrorResponse(
            ResponseValue::from_response(response).await?,
        )),
        500u16..=599u16 => Err(Error::ErrorResponse(
            ResponseValue::from_response(response).await?,
        )),
        _ => Err(Error::UnexpectedResponse(response)),
    }
}

And in turn, the CLI, which previously would only close with Connection lost. on errors prior to the server's accepting the protocol change, with this version of progenitor would now instead output, for example:

$ oxide instance serial console --project proj --instance inst
Connection closed: Error                                                                                                                                                                                                                                                       
Server error: Object (of type ByName("foo")) not found: instance

This fixes a bug wherein client code would be generated for a 2XX-range
response on WebSocket endpoints, which in turn would unnecessarily trip
an `assert!(response_types.len() <= 1)` in `extract_responses` when
explicit status codes are provided in the OpenAPI document.
(This also fixes a bug in such a scenario wherein *all* responses,
including HTTP errors, would be set to Upgrade type)

(As in dropshot#1548)
Comment on lines +476 to +481
// TODO: remove when no longer supporting older dropshot
// w/o explicit response codes for WebSocket endpoints:
|| (dropshot_websocket && status_code == OperationResponseStatus::Default)
{
// TODO (as above)
status_code = OperationResponseStatus::Code(101);
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's delete this now; I don't think we need to support the erroneous format.

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