[TEST] BasicCurlHttpTests.RepeatedCallerThreadCancelsAreClean is flaky

[dbarker]

The BasicCurlHttpTests.RepeatedCallerThreadCancelsAreClean test has been observed to fail sporadically in the Bazel valgrind CI workflow.

[ RUN      ] BasicCurlHttpTests.RepeatedCallerThreadCancelsAreClean
ext/test/http/curl_http_test.cc:689: Failure
Expected: (terminal_total) >= (20), actual: 14 vs 20

[  FAILED  ] BasicCurlHttpTests.RepeatedCallerThreadCancelsAreClean (501 ms)

Bazel valgrind test log (click to expand)

FAIL: //ext/test/http:curl_http_test (Exit 1) (see /home/runner/.cache/bazel/e6b8d6759295e14b76bc8cb98b604748/execroot/_main/bazel-out/k8-fastbuild/testlogs/ext/test/http/curl_http_test/test.log)
[8,840 / 8,982] 50 / 191 tests, 1 failed; Testing //sdk/test/metrics:all_tests; 86s processwrapper-sandbox ... (4 actions running)
==================== Test output for //ext/test/http:curl_http_test:
INFO: From Testing //ext/test/http:curl_http_test:
==14626== Memcheck, a memory error detector
==14626== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==14626== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info
==14626== Command: ext/test/http/curl_http_test
==14626== 
Running main() from gmock_main.cc
[==========] Running 26 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 26 tests from BasicCurlHttpTests
[ RUN      ] BasicCurlHttpTests.DoNothing
[       OK ] BasicCurlHttpTests.DoNothing (100 ms)
[ RUN      ] BasicCurlHttpTests.HttpRequest
[       OK ] BasicCurlHttpTests.HttpRequest (39 ms)
[ RUN      ] BasicCurlHttpTests.HttpResponse
[       OK ] BasicCurlHttpTests.HttpResponse (502 ms)
[ RUN      ] BasicCurlHttpTests.SendGetRequest
[       OK ] BasicCurlHttpTests.SendGetRequest (682 ms)
[ RUN      ] BasicCurlHttpTests.SendPostRequest
[       OK ] BasicCurlHttpTests.SendPostRequest (59 ms)
[ RUN      ] BasicCurlHttpTests.RequestTimeout
[       OK ] BasicCurlHttpTests.RequestTimeout (5513 ms)
[ RUN      ] BasicCurlHttpTests.CurlHttpOperations
[Error] File: ext/src/http/client/curl/http_operation_curl.cc:1343 Unexpected HTTP method
[       OK ] BasicCurlHttpTests.CurlHttpOperations (527 ms)
[ RUN      ] BasicCurlHttpTests.RetryPolicyEnabled
[       OK ] BasicCurlHttpTests.RetryPolicyEnabled (11 ms)
[ RUN      ] BasicCurlHttpTests.RetryPolicyDisabled
[       OK ] BasicCurlHttpTests.RetryPolicyDisabled (5 ms)
[ RUN      ] BasicCurlHttpTests.ExponentialBackoffRetry
[       OK ] BasicCurlHttpTests.ExponentialBackoffRetry (40 ms)
[ RUN      ] BasicCurlHttpTests.ACancelAfterTheResponseReportsOneOutcome
[       OK ] BasicCurlHttpTests.ACancelAfterTheResponseReportsOneOutcome (18 ms)
[ RUN      ] BasicCurlHttpTests.ACancelBeforeTheResponseReportsCancelled
[       OK ] BasicCurlHttpTests.ACancelBeforeTheResponseReportsCancelled (501 ms)
[ RUN      ] BasicCurlHttpTests.RetryJitterIsNotSharedAcrossThreads
[       OK ] BasicCurlHttpTests.RetryJitterIsNotSharedAcrossThreads (510 ms)
[ RUN      ] BasicCurlHttpTests.ResetMultiHandleWithASessionDoesNotDeadlock
[       OK ] BasicCurlHttpTests.ResetMultiHandleWithASessionDoesNotDeadlock (10 ms)
[ RUN      ] BasicCurlHttpTests.ACancelFromTheCallerThreadReportsCancelled
[       OK ] BasicCurlHttpTests.ACancelFromTheCallerThreadReportsCancelled (508 ms)
[ RUN      ] BasicCurlHttpTests.RepeatedCallerThreadCancelsAreClean
ext/test/http/curl_http_test.cc:689: Failure
Expected: (terminal_total) >= (20), actual: 14 vs 20

[  FAILED  ] BasicCurlHttpTests.RepeatedCallerThreadCancelsAreClean (501 ms)
[ RUN      ] BasicCurlHttpTests.SendGetRequestSync
[       OK ] BasicCurlHttpTests.SendGetRequestSync (13 ms)
[ RUN      ] BasicCurlHttpTests.HandlerRequestedCloseKeepsServerUsable
[       OK ] BasicCurlHttpTests.HandlerRequestedCloseKeepsServerUsable (11 ms)
[ RUN      ] BasicCurlHttpTests.SendGetRequestSyncTimeout
[       OK ] BasicCurlHttpTests.SendGetRequestSyncTimeout (5008 ms)
[ RUN      ] BasicCurlHttpTests.SendPostRequestSync
[       OK ] BasicCurlHttpTests.SendPostRequestSync (9 ms)
[ RUN      ] BasicCurlHttpTests.GetBaseUri
[       OK ] BasicCurlHttpTests.GetBaseUri (501 ms)
[ RUN      ] BasicCurlHttpTests.SendGetRequestAsyncTimeout
[       OK ] BasicCurlHttpTests.SendGetRequestAsyncTimeout (501 ms)
[ RUN      ] BasicCurlHttpTests.SendPostRequestAsync
[       OK ] BasicCurlHttpTests.SendPostRequestAsync (65 ms)
[ RUN      ] BasicCurlHttpTests.FinishInAsyncCallback
[       OK ] BasicCurlHttpTests.FinishInAsyncCallback (53 ms)
[ RUN      ] BasicCurlHttpTests.ElegantQuitQuick
[       OK ] BasicCurlHttpTests.ElegantQuitQuick (21 ms)
[ RUN      ] BasicCurlHttpTests.BackgroundThreadWaitMore
[       OK ] BasicCurlHttpTests.BackgroundThreadWaitMore (502 ms)
[----------] 26 tests from BasicCurlHttpTests (16241 ms total)

[----------] Global test environment tear-down
[==========] 26 tests from 1 test suite ran. (16284 ms total)
[  PASSED  ] 25 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] BasicCurlHttpTests.RepeatedCallerThreadCancelsAreClean

 1 FAILED TEST
  YOU HAVE 1 DISABLED TEST

==14626== 
==14626== HEAP SUMMARY:
==14626==     in use at exit: 0 bytes in 0 blocks
==14626==   total heap usage: 8,319 allocs, 8,319 frees, 2,172,157 bytes allocated
==14626== 
==14626== All heap blocks were freed -- no leaks are possible
==14626== 
==14626== For lists of detected and suppressed errors, rerun with: -s
==14626== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
================================================================================

[github-actions]

This issue is available for anyone to work on. Make sure to reference this issue in your pull request.
✨ Thank you for your contribution! ✨

[owent]

I took a look at this flake. Root cause: a race between the caller thread's
CancelSession() and the background thread reporting the connect failure. In
the losing iterations the request terminates as ConnectFailed — a terminal
state TerminalCountingHandler deliberately does not count — and no
Cancelled is dispatched at all.

Timeline of a lost iteration

1. SendRequest schedules the session; the background thread starts
   connecting to 127.0.0.1:19937. Nothing listens there, so the connection
   is refused almost immediately.
2. If the caller thread is descheduled between SendRequest and
   CancelSession (likely under valgrind, where guest threads are serialized
   and the CI host is heavily loaded), the background thread completes the
   whole failure cycle first: curl_multi_info_read →
   PerformCurlMessage(CURLE_COULDNT_CONNECT) → DispatchEvent(ConnectFailed)
   → Cleanup().
3. In Cleanup() the state is already ConnectFailed, so the self-dispatch of
   Cancelled (which only covers Created/Connecting/Connected/Sending) is
   skipped. The completion callback in Session::SendRequest then sees
   GetSessionState() != Response and WasAborted() == false (Abort() has
   not run yet), so it dispatches neither OnResponse nor
   OnEvent(Cancelled, ""). Cleanup() also clears async_data_->session,
   so the late CancelSession() → Abort() never even reaches
   ScheduleAbortSession.
4. The handler ends the iteration with terminal_count_ == 0 — it did receive
   a terminal notification (OnEvent(ConnectFailed)), but the handler only
   counts Cancelled and Response.

The observed numbers match: 14 vs 20 is 14 iterations where the cancel won
(each contributing exactly one counted Cancelled from the completion
callback, via CURLE_ABORTED_BY_CALLBACK → PerformCurlMessage →
Cleanup()) plus 6 where the connect failure won (contributing 0).

This is not a missing wait on the caller side

Session::FinishSession() → HttpOperation::Finish() blocks on
result_future until the background thread's Cleanup() has finished running
the completion callback (the promise is set right after the callback), and
FinishAllSessions() is a no-op here because ScheduleAbortSession /
CleanupSession have already removed the session from sessions_. So when
the test reads terminal_count_, the value is final — the missing counts
never arrive.

That makes this a flaw in the test's expectation rather than in the client's
synchronization: the assertion assumes every iteration ends in Cancelled,
but a request cancelled after it has already terminally failed correctly
reports ConnectFailed instead. (Reporting Cancelled for an
already-terminated session would also reintroduce the double terminal
notification tracked in #4360.)