[BUG] The curl HTTP client can report a request failed and still send it, and can deliver two terminal events

[thc1006]

Three places in Session::SendRequest where the shared curl client tells an exporter
something other than what it then does. This is ext/src/http/client/curl/, the production
client every HTTP exporter runs on, not the embedded test server whose scope was settled on
#4287.

All three are on main today.

1. A gzip failure reports the request as failed and then sends it anyway

http_client_curl.cc, inside the kGzip branch, which is compiled under
ENABLE_OTLP_COMPRESSION_PREVIEW, so this one reaches only builds with compression enabled:

    if (stream != Z_OK)
    {
      if (callback)
      {
        callback->OnEvent(SessionState::CreateFailed, zs.msg ? zs.msg : "");
      }
      is_session_active_.store(false, std::memory_order_release);
    }

    deflateEnd(&zs);

There is no return. Control falls through to curl_operation_.reset(new HttpOperation(...))
and SendAsync, so the exporter has already been told the request failed while the request
goes out.

The body is the worrying part. deflateInPlace compresses into the caller's buffer, and
only the success path adds Content-Encoding: gzip and resizes to max_size. A failure
partway through therefore leaves a buffer that may have been written in place, at its
original length, with no encoding header, and that is what gets sent.

What an exporter does with this depends on the exporter, but the shapes are all bad: report
failure and let a retry send the batch a second time, or report failure while the server
accepts something it cannot parse.

2. Cancelled and OnResponse can both fire for one operation

In the SendAsync completion lambda:

        if (operation.WasAborted())
        {
          callback->OnEvent(SessionState::Cancelled, "");
        }

        if (operation.GetSessionState() == SessionState::Response)
        {
          ...
          callback->OnResponse(*response);
        }

Two independent ifs, so an operation aborted after a response arrived delivers both. A
handler that treats either as terminal sees its export settle twice. That is the same shape
as #4338 on the Elasticsearch side, where the fix under review adds a first-writer-wins
guard in the exporter. The guard is needed in every consumer as long as the client can do
this.

3. One setup failure produces two terminal events

HttpOperation::SendAsync dispatches ConnectFailed and returns non-CURLE_OK
(http_operation_curl.cc). Back in Session::SendRequest, success is false, so the else
branch dispatches CreateFailed as well. One failure, two events, and a handler counting
terminal states counts two.

What would settle it

A contract on EventHandler: exactly one terminal event per SendRequest, and no bytes on
the wire after one has been delivered. Concretely that means returning after the gzip
failure, making the abort and response branches mutually exclusive, and letting the setup
path own its single event rather than both layers reporting.

Until then every consumer needs its own idempotence guard to be correct, which is a rule
nothing in the interface states.

I have not sent a patch. Three separate behaviour changes to a shared client is more than
belongs in one, and I would rather hear which of the three you want changed and whether the
EventHandler contract should be written down first.

[thc1006]

#4363 takes the second of the three, the one where I think there is only one answer: report
the response and drop the Cancelled, since the state only reaches Response when the
server actually answered, and telling a caller its accepted request was cancelled invites a
retry that writes the batch twice.

It turned out to be reproducible rather than a race. Cancelling from inside the handler's
Response event lands the abort in exactly the window that matters, because DispatchEvent
notifies the handler before it stores the new state and the completion callback runs after
both. On main the handler counts two terminal notifications for one request; with the
change it counts one.

Leaving the other two here. The gzip one has a real fork in it, whether a failed compression
should return without sending or send the body uncompressed, and the doubled setup failure
touches which layer owns the event. Both are yours to decide rather than mine to guess.

[marcalff]

Thanks for the bug reports and analysis.

[thc1006]

Update, and one of the two open questions has answered itself.

	where it stands on main at f6e4818
1. a gzip failure reports the request failed and sends it anyway	open, http_client_curl.cc:185-195
2. Cancelled and OnResponse both fire	fixed by #4363
3. one setup failure, two terminal events	open, http_operation_curl.cc:1449 plus http_client_curl.cc:239

Item 1 is not a fork after all

I left it as a choice between returning without sending and sending the body uncompressed. Sending it uncompressed is not on the table, because by the time the failure is visible there is no uncompressed body left.

deflateInPlace at http_client_curl.cc:70 is a local helper rather than zlib's, and it compresses straight into the caller's buffer. Exactly one of its failure exits happens before it starts writing:

  auto ret        = deflate(strm, Z_FINISH);
  if (ret == Z_STREAM_ERROR)
  {
    return ret;                                // :89, the only exit that leaves buf alone
  }

  auto have = strm->next_out - temp.data();
  if (have <= ...)
  {
    std::memcpy(buf, temp.data(), have);       // :99, from here buf is being overwritten
    strm->next_out = buf + have;
    have           = 0;
    while (ret == Z_OK)
    {
      ...
      ret = deflate(strm, Z_FINISH);
    }

Every failure after :99 returns with the buffer part compressed. body_.resize(max_size) and the Content-Encoding header are both on the success path only, so what falls through to SendAsync is the original length holding partly compressed bytes and no header saying so. A server either rejects it or, worse, does not.

So item 1 has one shape: return after the failure event rather than falling through, and clean the session up on the way out so it does not sit in sessions_ (CreateSession registers it, and nothing else would). Say the word and it goes out as its own change.

Item 3 still needs you to pick a layer

Two shapes, and I do not think either is obviously right from outside the project:

Move it up. SendAsync stops dispatching and only returns the code, and Session::SendRequest reports a single CreateFailed for everything that fails before the transfer is scheduled. Tidier boundary, at the cost of moving an event out of the operation that detected it.

Move it down. SendAsync keeps ConnectFailed and SendRequest stops adding CreateFailed on top. Smaller diff, but then no setup failure ever produces CreateFailed, which sits oddly with the name.

Either way it changes what EventHandler promises, which is the part I would rather not guess at.

One scheduling note

#4392, #4394 and #4395 are open against these same two files. Whichever shape you pick for either item, tell me and I will fit it around whatever has landed by then, rather than sending a fourth change that collides with the first three.