fix(http): bound the WinHTTP response-header wait, and stop docs triggering builds #5

Merged
jknapp merged 3 commits from fix/winhttp-response-header-timeout into main 2026-09-10 01:54:16 +00:00
3 changed files with 115 additions and 14 deletions
Showing only changes of commit 8888e57d08 - Show all commits
+22
View File
@@ -31,7 +31,29 @@ on:
# job is ordinary commits. # job is ordinary commits.
branches: branches:
- "**" - "**"
# Documentation-only changes cannot break a build, and this workflow is a
# full three-platform build (Windows included) behind a runner with
# capacity:1. Six of these fired for one afternoon of README/release-notes
# edits on 2026-09-09. Anything that feeds a build or a test is absent
# from this list on purpose -- release.yml and publish-release.sh only run
# on a `v*` tag, via release.yml's own trigger.
#
# Tradeoff: a docs-only push now shows NO status at all on the branch,
# rather than a green one. If a required-status check is ever added, these
# paths have to be reconsidered.
paths-ignore:
- "**.md"
- "LICENSE"
- "NOTICE"
- ".gitea/workflows/release.yml"
- ".gitea/scripts/publish-release.sh"
pull_request: pull_request:
paths-ignore:
- "**.md"
- "LICENSE"
- "NOTICE"
- ".gitea/workflows/release.yml"
- ".gitea/scripts/publish-release.sh"
jobs: jobs:
linux: linux:
+33
View File
@@ -116,6 +116,39 @@ public:
WinHttpSetTimeouts(session.get(), static_cast<int>(timeout), static_cast<int>(timeout), WinHttpSetTimeouts(session.get(), static_cast<int>(timeout), static_cast<int>(timeout),
static_cast<int>(timeout), static_cast<int>(timeout)); static_cast<int>(timeout), static_cast<int>(timeout));
// WinHttpSetTimeouts' receive parameter maps to
// WINHTTP_OPTION_RECEIVE_TIMEOUT, which Microsoft documents as a
// PER-PACKET Winsock-layer read timeout ("applies to fetching each
// packet of data off the socket"), not a deadline on the response.
// The wait for the response HEADERS is a *separate* option,
// WINHTTP_OPTION_RECEIVE_RESPONSE_TIMEOUT ("to wait to receive all
// response headers to a request"), which WinHttpSetTimeouts does not
// touch and which defaults to 90 SECONDS. Without this call a server
// that accepts, reads the request and then stalls can hold this
// thread for a minute and a half regardless of request.timeout_ms --
// exactly the "blocking an OBS thread indefinitely" failure
// testPlatformBackendTimeout exists to prevent, and the likely
// mechanism behind that test's intermittent Windows failures.
//
// Caveat, also documented: this timeout "is checked only when data is
// received from the socket", so it bounds the wait but does not
// guarantee a hard deadline. A guaranteed deadline needs a watchdog
// thread calling WinHttpCloseHandle; not done here.
//
// Guarded because the constant postdates some Windows SDK headers; a
// toolchain without it keeps the previous (90s default) behaviour
// rather than failing to build.
#ifdef WINHTTP_OPTION_RECEIVE_RESPONSE_TIMEOUT
DWORD response_timeout = timeout;
// Return value deliberately unchecked: a rejected option leaves the
// documented default in place, which is degraded but still correct
// behaviour, and there is no logging sink in this layer to report it
// to. The timeout probe in test_api_client.cpp is what would catch a
// regression here.
WinHttpSetOption(session.get(), WINHTTP_OPTION_RECEIVE_RESPONSE_TIMEOUT, &response_timeout,
sizeof(response_timeout));
#endif
Handle connect(WinHttpConnect(session.get(), host, parts.nPort, 0)); Handle connect(WinHttpConnect(session.get(), host, parts.nPort, 0));
if (!connect) { if (!connect) {
response.network_error = lastErrorMessage("WinHttpConnect"); response.network_error = lastErrorMessage("WinHttpConnect");
+50 -4
View File
@@ -16,6 +16,7 @@ You may obtain a copy of the License at
// three runners rather than assumed to work. // three runners rather than assumed to work.
#include <chrono> #include <chrono>
#include <cstdio>
#include <memory> #include <memory>
#include <string> #include <string>
#include <thread> #include <thread>
@@ -422,8 +423,32 @@ void testPlatformBackendTimeout()
{ {
// A server that accepts and then stalls. The plugin must give up on its // A server that accepts and then stalls. The plugin must give up on its
// own timeout rather than blocking an OBS thread indefinitely. // own timeout rather than blocking an OBS thread indefinitely.
sttest::LoopbackServer server([](const std::string &) { //
std::this_thread::sleep_for(std::chrono::seconds(5)); // INSTRUMENTED (2026-09-09) while chasing an intermittent Windows-only
// failure: on roughly 2 of 6 CI runs both assertions below fail together,
// meaning the request waited out the full 5s stall and returned 200 --
// the timeout did not fire at all. Same failure seen on 2026-09-07
// (job 5834) and 2026-09-09 (job 5911), on identical code that passed on
// other runs, so it is not a code change that caused it.
//
// The probe runs kProbes times and prints one line per attempt so a
// single CI run yields a failure RATE and the WinHTTP error code, rather
// than one bit. `ST_ASSERT` records and continues, so every attempt is
// reported even when one fails. Remove the loop and this comment once the
// mechanism is understood and fixed.
constexpr int kProbes = 5;
constexpr long long kStallMs = 5000;
constexpr long kTimeoutMs = 700;
int timed_out = 0;
for (int i = 0; i < kProbes; ++i) {
// A FRESH server per attempt, deliberately. `LoopbackServer` accepts
// and handles one connection at a time on a single thread, so reusing
// one server across attempts would leave attempts 2..n sitting in the
// accept backlog -- a different scenario (never accepted) from the one
// that fails on Windows (accepted, request read, then stalled).
sttest::LoopbackServer server([kStallMs](const std::string &) {
std::this_thread::sleep_for(std::chrono::milliseconds(kStallMs));
return sttest::httpResponse(200, "OK", R"({"slots":[]})"); return sttest::httpResponse(200, "OK", R"({"slots":[]})");
}); });
ST_ASSERT(server.valid()); ST_ASSERT(server.valid());
@@ -431,13 +456,34 @@ void testPlatformBackendTimeout()
std::shared_ptr<HttpClient> http(createPlatformHttpClient()); std::shared_ptr<HttpClient> http(createPlatformHttpClient());
HttpRequest request; HttpRequest request;
request.url = server.baseUrl() + "/api/obs/main-room/slots?key=k"; request.url = server.baseUrl() + "/api/obs/main-room/slots?key=k";
request.timeout_ms = 700; request.timeout_ms = kTimeoutMs;
const auto start = std::chrono::steady_clock::now(); const auto start = std::chrono::steady_clock::now();
const HttpResponse response = http->send(request); const HttpResponse response = http->send(request);
const auto elapsed = std::chrono::steady_clock::now() - start; const auto elapsed = std::chrono::steady_clock::now() - start;
const long long ms = std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count();
const bool gave_up = !response.ok() && ms < 4000;
if (gave_up)
++timed_out;
// Always printed, pass or fail: elapsed time and the backend's own
// error string (which carries GetLastError on Windows) are the
// evidence. requests_seen separates "the client never reached the
// server" (0) from "the server read the request and the client then
// waited it out" (1).
std::fprintf(stderr,
" [timeout-probe %d/%d] elapsed=%lldms ok=%d status=%ld "
"requests_seen=%d network_error='%s' -> %s\n",
i + 1, kProbes, ms, response.ok() ? 1 : 0, response.status,
server.requestCount(), response.network_error.c_str(),
gave_up ? "gave up (expected)" : "WAITED OUT THE STALL");
ST_ASSERT(!response.ok()); ST_ASSERT(!response.ok());
ST_ASSERT(std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count() < 4000); ST_ASSERT(ms < 4000);
}
std::fprintf(stderr, " [timeout-probe] %d/%d attempts honoured the %ldms timeout\n",
timed_out, kProbes, kTimeoutMs);
} }
} // namespace } // namespace