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

[thc1006]

Four places in the shared curl client where it 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.

Two of the four are fixed on main. Two are still live.

	status on f77c1a5c
1. gzip failure reports failed and sends anyway	fixed by #4457, the branch now returns
2. Cancelled and OnResponse both fire	fixed by #4392, the branches are else if
3. one setup failure, two terminal events	still present
4. Cancelled twice for one cancelled transfer	still present, measured below

The original three are kept below so the reports stay readable next to the fixes.

1. A gzip failure reports the request as failed and then sends it anyway (fixed, #4457)

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 (fixed, #4392)

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 (still on main)

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.

4. One cancelled transfer delivers two Cancelled events (still on main, measured)

#4392 fixed 2 by making the two arms of the completion callback mutually exclusive. That separates a response from a cancel. It does not separate a cancel from a cancel, because the first one is dispatched before the callback runs.

HttpOperation::Cleanup() dispatches for a transfer that had got as far as Created, Connecting, Connected or Sending:

  switch (GetSessionState())
  {
    case SessionState::Created:
    case SessionState::Connecting:
    case SessionState::Connected:
    case SessionState::Sending: {
      const char *message = GetCurlErrorMessage(last_curl_result_);
      DispatchEvent(SessionState::Cancelled, message);   // first
      break;
    }

DispatchEvent calls OnEvent and then assigns session_state_ = type, so the operation's state is now Cancelled. Cleanup() goes on to run the completion callback, and the else if from #4392 is reached precisely because the if above it can no longer match:

  if (operation.GetSessionState() == SessionState::Response)   // false, Cleanup just wrote Cancelled
  { ... }
  else if (operation.WasAborted())
  {
    callback->OnEvent(SessionState::Cancelled, "");            // second
  }

The two are distinguishable at the handler: the first carries a curl message, the second an empty reason.

Measured on f77c1a5c, using the case already in the tree. BasicCurlHttpTests.RepeatedCallerThreadCancelsAreClean runs twenty cancelled transfers and sums the terminal events:

PROBE terminal_total=40 over 20 iterations, mean=2
[       OK ] BasicCurlHttpTests.RepeatedCallerThreadCancelsAreClean (501 ms)

Exactly two per transfer, every time. The case is green because its assertion is EXPECT_GE(terminal_total, 20), a lower bound of one each, and its own comment says the count belongs to this issue.

This matters for the exactly-once contract below: a consumer that dedupes on "first terminal event wins" is fine, but one that counts them, or that treats a second Cancelled as a second export settling, is not.

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. Two of the concrete steps are done: the gzip branch returns and the abort and response branches are mutually exclusive. What is left is letting one layer own each terminal event rather than two reporting it, which is the same shape in 3 and in 4. A test for it has to count events rather than assert a lower bound, since both remaining paths deliver one correct event and one extra.

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.

---

Edited 2026-09-22. The opening said all three were on main, which stopped being true when #4392 and #4457 merged. Re-checked against f77c1a5c: the gzip branch now says the request does not go out, the abort and response branches are else if, and SendAsync still dispatches ConnectFailed before Session::SendRequest dispatches CreateFailed. A fourth path was added the same day: #4392's else if does not separate a cancel from a cancel, and the count is measured rather than argued.

[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.

[thc1006]

Correcting my own item 2, and confirming the other two still stand at 3fb1d31.

Item 2 is fixed. #4392 landed as a31138b and made the two branches exclusive, so an operation that was answered is reported as answered and an abort after a response no longer delivers both:

        if (operation.GetSessionState() == SessionState::Response)
        {
          ...
          callback->OnResponse(*response);
        }
        else if (operation.WasAborted())
        {
          callback->OnEvent(SessionState::Cancelled, "");
        }

Item 1 and item 3 are unchanged. The gzip branch still has no return after reporting CreateFailed, so the request still goes out, and deflateInPlace still writes into the caller's buffer while only the success path resizes and adds Content-Encoding. And one Setup() failure still produces ConnectFailed from SendAsync followed by CreateFailed from Session::SendRequest.

Worth being precise about what the #4404 work does and does not do here, since it touches the same lines. It gives the constructor a result and makes SendAsync refuse on it before Setup() runs, so a construction failure produces exactly one terminal event, and there is a case that counts it. It does nothing for either of the two above: a Setup() failure still takes the path that reports twice, and the gzip branch is untouched. So #4404 does not close this, and the case there claims exactly-once for construction failures only.

One thing that came out of the same work and belongs here rather than there. Because the constructor now has a failure to report and does not dispatch it, a caller using HttpOperation directly, which is a concrete type in an installed header, learns about a failed curl_easy_init or header list from the return code and not from the EventHandler it passed in. Through Session::SendRequest it arrives once, as CreateFailed. That is a coherent answer if Session is the only supported owner of asynchronous notification, and the asymmetry is that the constructor does dispatch Created on the way out when it succeeds. Which way that should go looks like part of the same contract this issue is asking for, so I have not guessed at it.

Still not sending a patch for the three. Which of them you want changed, and whether the EventHandler contract gets written down first, is the part I would rather hear before touching a client every exporter runs on.