From 94b48e10bc223907cdf917e52292f2913ed20571 Mon Sep 17 00:00:00 2001 From: KeviM Date: Fri, 25 Sep 2026 12:56:25 -0700 Subject: [PATCH] fix(http): count batch orders without copying or over-reading the body --- CHANGELOG.md | 8 ++++++ src/http/rate_limited_transport.cpp | 5 ++-- tests/CMakeLists.txt | 5 ++-- tests/README.md | 1 + tests/test_allocation_budgets.cpp | 44 +++++++++++++++++++++++++++++ tests/test_transports.cpp | 36 +++++++++++++++++++++++ 6 files changed, 95 insertions(+), 4 deletions(-) create mode 100644 tests/test_allocation_budgets.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index d2e80d8..d4dd899 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed + +- `RateLimitedTransport` counted a batch's orders past the end of the body + it was given. A truncated body was read one byte beyond its end, and a + view over the start of a longer buffer was counted from the bytes after + it. Counting also no longer copies each order: 20 orders now take 6 + allocations instead of 26. + ## [0.6.1] - 2026-09-25 ### Fixed diff --git a/src/http/rate_limited_transport.cpp b/src/http/rate_limited_transport.cpp index e49500f..49dcf25 100644 --- a/src/http/rate_limited_transport.cpp +++ b/src/http/rate_limited_transport.cpp @@ -10,7 +10,7 @@ namespace http_detail { // Glaze reflection needs a type with linkage, so this is not in an unnamed namespace. struct BatchBody { - std::vector orders; + std::vector orders; // Only the count matters, so nothing is copied. }; } // namespace http_detail @@ -23,7 +23,8 @@ std::size_t batch_items(std::string_view path, std::string_view body) { return 1; } http_detail::BatchBody batch; - constexpr glz::opts options{.error_on_unknown_keys = false}; + // `body` is a view, so no terminator is guaranteed after it. + constexpr glz::opts options{.null_terminated = false, .error_on_unknown_keys = false}; if (glz::read(batch, body) || batch.orders.empty()) { return 1; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a647b9a..c792151 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -37,8 +37,9 @@ gtest_discover_tests(kalshi_tests DISCOVERY_MODE PRE_TEST DISCOVERY_TIMEOUT 30) # Its own binary, because it replaces operator new for the whole process. if(TARGET kalshi_allocation_counter) - add_executable(kalshi_allocation_tests test_allocation_counter.cpp) - target_link_libraries(kalshi_allocation_tests PRIVATE kalshi_allocation_counter + add_executable(kalshi_allocation_tests test_allocation_counter.cpp + test_allocation_budgets.cpp) + target_link_libraries(kalshi_allocation_tests PRIVATE kalshi_allocation_counter kalshi::kalshi GTest::gtest_main) kalshi_configure_target(kalshi_allocation_tests) gtest_discover_tests(kalshi_allocation_tests DISCOVERY_MODE PRE_TEST) diff --git a/tests/README.md b/tests/README.md index 7dbda03..38d8452 100644 --- a/tests/README.md +++ b/tests/README.md @@ -19,6 +19,7 @@ memory. | `test_version.cpp` | `kalshi::VERSION` and its components | | `parse_benchmark.cpp` | A coarse throughput guard, run as its own test | | `test_allocation_counter.cpp` | The `operator new` counter in `support/`, in its own binary | +| `test_allocation_budgets.cpp` | Hot paths whose allocations must not grow with the size of each item | | `test_ws_messages.cpp` | Every example frame in the AsyncAPI spec (generated) | | `test_ws_frames.cpp` | Control frames, discriminated types, nulls, unknown values | | `test_ws_subscriptions.cpp` | Command frames, held commands, resubscribing, gaps | diff --git a/tests/test_allocation_budgets.cpp b/tests/test_allocation_budgets.cpp new file mode 100644 index 0000000..62a4393 --- /dev/null +++ b/tests/test_allocation_budgets.cpp @@ -0,0 +1,44 @@ +// Allocation budgets for hot paths. Each test compares two calls that should +// allocate the same, so it holds on every standard library. + +#include "kalshi/rate_limit.hpp" + +#include +#include +#include +#include + +#include "support/allocation_counter.hpp" + +namespace { + +using kalshi::test::AllocationProbe; + +std::string batch_of(int orders, std::string_view order) { + std::string body = R"({"orders":[)"; + for (int i = 0; i < orders; ++i) { + body += i == 0 ? "" : ","; + body += order; + } + body += "]}"; + return body; +} + +std::uint64_t cost_allocations(const kalshi::RateLimitedTransport& transport, + const std::string& body) { + const AllocationProbe probe; + (void)transport.cost(kalshi::HttpMethod::POST, "/portfolio/events/orders/batched", body); + return probe.count().count; +} + +TEST(AllocationBudget, CountingABatchDoesNotCopyItsOrders) { + const kalshi::RateLimitedTransport transport(nullptr, kalshi::RateLimitConfig{}); + const std::string empty_orders = batch_of(20, "{}"); + const std::string real_orders = + batch_of(20, R"({"ticker":"KXHIGHNY-26SEP25-T75","side":"yes","action":"buy","count":10,)" + R"("yes_price":42,"client_order_id":"5f0c7c1e-8d5a-4f4e-9b1a-2f6d3c9e7a10"})"); + + EXPECT_EQ(cost_allocations(transport, real_orders), cost_allocations(transport, empty_orders)); +} + +} // namespace diff --git a/tests/test_transports.cpp b/tests/test_transports.cpp index 8bea5a4..79b60dd 100644 --- a/tests/test_transports.cpp +++ b/tests/test_transports.cpp @@ -256,6 +256,42 @@ TEST(RateLimitedTransport, FailsFastInsteadOfWaitingPastMaxWait) { EXPECT_EQ(inner->calls, 1); } +TEST(RateLimitedTransport, CountsBatchItemsWhateverTheyContain) { + const kalshi::RateLimitedTransport transport(std::make_shared(), + kalshi::RateLimitConfig{}); + const std::string body = + R"({"orders":[{"ticker":"A,]}","tags":[1,[2,3]],"note":"say \"hi\""},{},)" + R"({"ticker":"B","nested":{"orders":[{},{}]}}],"extra":{"orders":[]}})"; + + EXPECT_DOUBLE_EQ( + transport.cost(kalshi::HttpMethod::POST, "/portfolio/events/orders/batched", body), 30.0); +} + +TEST(RateLimitedTransport, CountsOnlyTheBodyItIsGiven) { + // The view stops before the closing brace, so it is truncated JSON and + // costs one unit. Reading past the view would find the brace and count two. + const std::string buffer = R"({"orders":[{},{}]})"; + const kalshi::RateLimitedTransport transport(std::make_shared(), + kalshi::RateLimitConfig{}); + + EXPECT_DOUBLE_EQ(transport.cost(kalshi::HttpMethod::POST, "/portfolio/events/orders/batched", + std::string_view{buffer.data(), buffer.size() - 1}), + 10.0); +} + +TEST(RateLimitedTransport, CountsBatchItemsWithoutReadingPastTheBody) { + // A truncated body that fills its buffer exactly, so a read past the view is + // a read past the allocation, which the sanitizer build reports. + const std::string_view text = R"({"orders":[{"ticker":"A"},{"ticker":"B"})"; + const std::vector buffer(text.begin(), text.end()); + const kalshi::RateLimitedTransport transport(std::make_shared(), + kalshi::RateLimitConfig{}); + + EXPECT_DOUBLE_EQ(transport.cost(kalshi::HttpMethod::POST, "/portfolio/events/orders/batched", + std::string_view{buffer.data(), buffer.size()}), + 10.0); +} + TEST(RateLimitedTransport, BatchesLargerThanTheBucketFailClearly) { const std::shared_ptr inner = std::make_shared(); const kalshi::RateLimitedTransport transport(inner, kalshi::RateLimitConfig{});