test: Fixup MAX_BODY_SIZE http throttling test #36324

maflcko commented at 7:17 AM on September 24, 2026: member

The check_slow_read_throttle test asserts that throttling happens during a hard-coded time limit, via a hard-coded tries limit.

This is mostly fine, but can intermittently fail on slow CPUs or when using unoptimized sanitizers.

Fix it by waiting for a time scaled by --timeout-factor, without a hard-coded tries limit. Also, retain the hard-coded 5s sleep to "verify" throttling happened.

DrahtBot added the label Tests on Sep 24, 2026

DrahtBot commented at 7:17 AM on September 24, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36324.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	pinheadmz, winterrdog, davidgumberg, willcl-ark

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

[test/functional/interface_http.py] assert count < num_req, f"Server handled the whole batch of {num_req}: nothing was throttled" -> use assert_greater_than(num_req, count) instead of a bare comparison assert

maflcko added the label Needs Backport (32.x) on Sep 24, 2026

maflcko added this to the milestone 32.0 on Sep 24, 2026

maflcko commented at 7:19 AM on September 24, 2026: member

To reproduce the failure, one can simulate a slow CPU on large requests via:

diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 6c9a569d42..587a5b4679 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -600,2 +600,6 @@ void HTTPRequest::WriteReply(HTTPStatusCode status, std::span<const std::byte> r
 
+    if (reply_body.size() > 1'000'000) {
+        UninterruptibleSleep(5123ms);
+    }
+
     if (std::shared_ptr client{m_client.lock()}) {

The failure will be:

./test/functional/interface_http.py", line 847, in check_slow_read_throttle
    assert tries > 0, f"Progress failed to stall after {count} requests were handled."
           ^^^^^^^^^
AssertionError: Progress failed to stall after 5 requests were handled.

DrahtBot added the label CI failed on Sep 24, 2026

maflcko marked this as a draft on Sep 24, 2026

test: Fixup MAX_BODY_SIZE http throttling test fa2c62036b

maflcko marked this as ready for review on Sep 24, 2026

maflcko force-pushed on Sep 24, 2026

fanquake commented at 8:51 AM on September 24, 2026: member

cc @pinheadmz @hodlinator

DrahtBot removed the label CI failed on Sep 24, 2026

in test/functional/interface_http.py:851 in fa2c62036b

 855 | -                time.sleep(5)
 856 | +                return False
 857 | +
 858 | +        # Large enough interval, to ensure throttling occurred, rather than
 859 | +        # merely slow JSON serialization.
 860 | +        self.wait_until(progress_stalled, check_interval=5)

pinheadmz commented at 8:38 PM on September 24, 2026:

Non blocking, but In #36303 I put a try/catch around this so if the timeout expires I still get the error message I like ("Progress failed to stall after...")

maflcko commented at 8:51 PM on September 24, 2026:

Heh, yeah, I don't like those extra logic steps just to print a seemingly nicer error message. Seems harder to read the code then. Also, when it fails, it is already clear that the timeout is due the the stall not happening:

./bld-cmake/test/functional/interface_http.py", line 851, in check_slow_read_throttle
    self.wait_until(progress_stalled, check_interval=5)
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
./test/functional/test_framework/util.py", line 451, in wait_until_helper_internal
    raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
...

Seems like a common pattern used in all tests.

Also, when a test fails, one has to fully understand the test code and source code anyway, so trying to make the error messages minimally nicer probably doesn't help with that.

maflcko commented at 7:13 AM on September 25, 2026:

Same for the other asserts in this file :sweat_smile: Basically what the DrahtBot comment says:

[test/functional/interface_http.py] assert count < num_req, f"Server handled the whole batch of {num_req}: nothing was throttled" -> use assert_greater_than(num_req, count) instead of a bare comparison assert

The latter seems cleaner, more idiomatic in this codebase and possibly more useful, as it prints both sides on failure, not just num_req. (Recall there could also be a bug in the test code and count is wrong).

But I didn't want to re-write this file here and just keep this as minimal fixup :)

pinheadmz approved