Merge TASK-072: arena-allocated unescape on the warm path

Routes GET-argument unescape into the per-connection arena so the warm-path
zero-allocation criterion holds even when a user unescaper is registered.
Adds shared unescape_helpers.hpp, an arena-backed sink in build_request_args,
a per-thread scratch buffer for the ABI-locked callback path, unit tests
pinning zero global operator new calls, and bench_warm_path variants (5)/(6).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: etr

specs/architecture/04-components/http-request.md

@@ -19,6 +19,7 @@
 
 **Key design notes:**
 - The arena allocator is plumbed through `webserver_impl` → connection state → `http_request` constructor. The user does not see it; it is an internal optimization.
+  - GET-argument population (`http_request_impl::build_request_args`) writes the unescaped value directly into the per-connection arena (TASK-072). The standard `%HH` / `+`→space transformation runs in-place on the arena-backed `pmr::string`; when a user-registered unescaper is set, a per-thread reusable `std::string` scratch buffer adapts the ABI-locked `void(std::string&)` callback signature without a per-request global-heap allocation. Pinned by `test/unit/http_request_unescape_arena_test.cpp` (zero global `operator new` calls during the warm cycle) and exercised by `bench_warm_path` variants (5) and (6).
 - Containers returned by `get_*()` reference impl-owned storage; the request must outlive any view derived from it. Documented as a lifetime contract.
 - `gnutls_session_t` (raw GnuTLS handle) is not exposed publicly. Users wanting custom TLS introspection use the high-level `get_client_cert_*` accessors. The handle remains accessible via friend access from internal code.
 

specs/tasks/M7-v2-cleanup/TASK-072.md

@@ -8,10 +8,10 @@
 `src/detail/http_request_impl.cpp:202-206` is an acknowledged TASK-018 deferral: the warm-path "0-alloc" criterion explicitly does not apply when a user unescaper is registered, because the unescape call still allocates a `std::string`. Route the unescape output into the per-connection arena (TASK-016) so the criterion holds unconditionally.
 
 **Action Items:**
-- [ ] Extend the per-connection arena (from TASK-016) with an `unescape_into(string_view in, void* sink) -> string_view` helper, or have the arena expose a scratch-buffer API the unescape can write into.
-- [ ] Refactor the unescape call site at `http_request_impl.cpp:202-206` to allocate into the arena. The returned `string_view` is valid for the lifetime of the request, matching the TASK-018 lifetime documentation.
-- [ ] Update the inline TASK-018 deferral comment to reflect that arena unescape is now wired.
-- [ ] Extend `test/bench_warm_path.cpp` (TASK-058) with an unescape variant: warm GET with `%2F` in the path. Pin "no allocations under heap profiler" as in TASK-058.
+- [x] Extend the per-connection arena (from TASK-016) with an `unescape_into(string_view in, void* sink) -> string_view` helper, or have the arena expose a scratch-buffer API the unescape can write into.
+- [x] Refactor the unescape call site at `http_request_impl.cpp:202-206` to allocate into the arena. The returned `string_view` is valid for the lifetime of the request, matching the TASK-018 lifetime documentation.
+- [x] Update the inline TASK-018 deferral comment to reflect that arena unescape is now wired.
+- [x] Extend `test/bench_warm_path.cpp` (TASK-058) with an unescape variant: warm GET with `%2F` in the path. Pin "no allocations under heap profiler" as in TASK-058.
 
 **Dependencies:**
 - Blocked by: TASK-016 (arena, Done), TASK-018 (string_view getters, Done), TASK-058 (warm-path bench infra, Done)
@@ -27,4 +27,4 @@
 **Related Requirements:** PRD-REQ-REQ-001 (const-correctness + zero-alloc)
 **Related Decisions:** DR-003b (arena)
 
-**Status:** Backlog
+**Status:** Done

specs/tasks/M7-v2-cleanup/_index.md

@@ -36,7 +36,7 @@ TASK-093).
 | TASK-069 | Remove transitional two-arg `http_request_impl` constructor | MED | S | Done |
 | TASK-070 | Migrate `hook_table_` to `std::atomic<std::shared_ptr<T>>` | MED | M | Backlog |
 | TASK-071 | Wire `install_not_found_alias_` stub and remove dead `lambda_handler` arm | MED | S | Done |
-| TASK-072 | Arena-allocated unescape on the warm path | MED | M | Backlog |
+| TASK-072 | Arena-allocated unescape on the warm path | MED | M | Done |
 | TASK-073 | Revisit libmicrohttpd v0.99 unescape workaround | LOW | S | Backlog |
 | TASK-074 | Gate DEBUG raw-body printing behind explicit opt-in | LOW (sec) | S | Backlog |
 | TASK-075 | Propagate `wait_for_server_ready` to sibling hooks integ tests | HIGH | M | Backlog |

specs/unworked_review_issues/2026-06-08_112046_task-072.md

@@ -29,7 +29,7 @@ libhttpserver_la_SOURCES = string_utilities.cpp webserver.cpp http_utils.cpp fil
 # noinst_HEADERS: shipped in the tarball but NEVER installed under $prefix/include.
 # Detail headers (httpserver/detail/*.hpp) live here so they cannot leak to
 # downstream consumers — the public surface comes in through <httpserver.hpp>.
-noinst_HEADERS = httpserver/string_utilities.hpp httpserver/detail/modded_request.hpp httpserver/detail/http_endpoint.hpp httpserver/detail/body.hpp httpserver/detail/webserver_impl.hpp httpserver/detail/webserver_impl_dispatch.hpp httpserver/detail/connection_state.hpp httpserver/detail/secure_zero.hpp httpserver/detail/http_request_impl.hpp httpserver/detail/resource_hook_table.hpp httpserver/detail/route_entry.hpp httpserver/detail/lambda_resource.hpp httpserver/detail/radix_tree.hpp httpserver/detail/route_cache.hpp httpserver/detail/route_tier.hpp gettext.h
+noinst_HEADERS = httpserver/string_utilities.hpp httpserver/detail/modded_request.hpp httpserver/detail/http_endpoint.hpp httpserver/detail/body.hpp httpserver/detail/webserver_impl.hpp httpserver/detail/webserver_impl_dispatch.hpp httpserver/detail/connection_state.hpp httpserver/detail/secure_zero.hpp httpserver/detail/http_request_impl.hpp httpserver/detail/resource_hook_table.hpp httpserver/detail/route_entry.hpp httpserver/detail/lambda_resource.hpp httpserver/detail/radix_tree.hpp httpserver/detail/route_cache.hpp httpserver/detail/route_tier.hpp httpserver/detail/unescape_helpers.hpp gettext.h
 nobase_include_HEADERS = httpserver.hpp httpserver/body_kind.hpp httpserver/cookie.hpp httpserver/constants.hpp httpserver/create_webserver.hpp httpserver/create_test_request.hpp httpserver/webserver.hpp httpserver/webserver_routes.hpp httpserver/webserver_websocket.hpp httpserver/webserver_hooks.hpp httpserver/websocket_handler.hpp httpserver/http_utils.hpp httpserver/ip_representation.hpp httpserver/file_info.hpp httpserver/http_request.hpp httpserver/http_request_auth.hpp httpserver/http_response.hpp httpserver/http_resource.hpp httpserver/feature_unavailable.hpp httpserver/iovec_entry.hpp httpserver/http_arg_value.hpp httpserver/http_method.hpp httpserver/hook_phase.hpp httpserver/hook_action.hpp httpserver/hook_handle.hpp httpserver/hook_context.hpp
 
 AM_CXXFLAGS += -fPIC -Wall

src/Makefile.am

@@ -39,6 +39,7 @@
 
 #include "httpserver/detail/connection_state.hpp"
 #include "httpserver/detail/http_request_impl.hpp"
+#include "httpserver/detail/unescape_helpers.hpp"
 #include "httpserver/http_utils.hpp"
 
 namespace httpserver {
@@ -155,6 +156,59 @@ const http::header_view_map& http_request_impl::ensure_headerlike_cache(MHD_Valu
     return *cache;
 }
 
+namespace {
+
+// TASK-072: arena-routed unescape. The caller passes an arena-backed
+// pmr::string already holding the raw bytes; we run the unescape
+// transformation directly on the pmr::string's storage so no
+// global-heap allocation occurs on the warm path.
+//
+// Default path: calls httpserver::detail::unescape_buf_raw (shared
+// helper from unescape_helpers.hpp -- same algorithm as the
+// http_unescape wrapper in http_utils.cpp, one truth-source).
+//
+// User-callback path: the public callback signature is
+// `void(std::string&)` (ABI-locked), so we route through a
+// thread_local std::string (thread_value) whose capacity amortises
+// across all requests on the same worker thread.
+//
+// Amortisation contract: zero global-heap allocations on the warm
+// path per thread, *after* thread_value has grown to the peak value
+// length seen on that thread. The first call on a new thread, or the
+// first call that sees a value longer than any previous one, triggers
+// exactly one global-heap allocation to grow thread_value's buffer;
+// all subsequent calls at or below that length are allocation-free.
+// (performance-reviewer-iter1-2)
+//
+// Arena-waste note: when the user callback grows the value
+// (thread_value.size() > original value.size()), value's original
+// arena allocation is abandoned in the monotonic arena until
+// reset_arena(). This is an accepted trade-off of the ABI-locked
+// void(std::string&) signature; the arena is reset per request so
+// the waste is bounded by the request's total value bytes.
+// (performance-reviewer-iter1-3)
+inline void unescape_in_arena(std::pmr::string& value,
+                              unescaper_ptr user_fn) {
+    if (value.empty()) return;
+    if (user_fn == nullptr) {
+        // Default %HH / '+' decode: run in-place on the arena
+        // buffer, then truncate to the new size.
+        const std::size_t new_size = httpserver::detail::unescape_buf_raw(
+            value.data(), value.size());
+        value.resize(new_size);
+        return;
+    }
+    // User-callback path: route through a per-thread reusable
+    // std::string scratch buffer so the warm-path cost is zero
+    // global-heap allocations after the first call on this thread.
+    thread_local std::string thread_value;
+    thread_value.assign(value.data(), value.size());
+    user_fn(thread_value);
+    value.assign(thread_value.data(), thread_value.size());
+}
+
+}  // namespace
+
 MHD_Result http_request_impl::build_request_args(void* cls, MHD_ValueKind kind,
                                                  const char* key, const char* arg_value) {
     // Parameters needed to respect MHD interface, but not used in the implementation.
@@ -190,28 +244,10 @@ MHD_Result http_request_impl::build_request_args(void* cls, MHD_ValueKind kind,
     }
     aa->accumulated_bytes += this_pair_bytes;
 
-    // Unescape into a temporary std::string (the C-style unescaper is
-    // string-typed). This causes one global-heap allocation (the temp) even
-    // on the warm arena path, followed by a second copy from the temp into
-    // the arena-backed pmr::string via emplace_back below. The warm-path
-    // zero-upstream-allocs guarantee therefore does NOT hold for requests
-    // with GET arguments when an unescaper is active.
-    //
-    // Tracked as TASK-018: route the unescape output directly into a
-    // pmr::string using the arena allocator, eliminating both the temp and
-    // the second copy. Until then, the acceptance criterion '0 bytes
-    // global-heap allocation from impl construction on warm connection'
-    // applies to construction and arena-backed containers only -- not to
-    // argument population when an unescaper is registered.
-    // (performance-reviewer-iter1-1: acknowledged deferral)
-    std::string value(val_sv);
-    http::base_unescaper(&value, aa->unescaper);
-
-    // Reuse the iterator from the hoisted find above: insert if missing,
-    // then append the (possibly unescaped) value. The key is stored as
-    // pmr::string in the map's allocator domain; the value vector is
-    // allocator-constructed in place via the same allocator (scoped
-    // propagation gives nested pmr::strings the right allocator too).
+    // Arena-routed unescape: see unescape_in_arena() above for path details.
+    // The returned pmr::string's data is owned by the arena and lives until
+    // connection_state::reset_arena() runs (request completion), matching
+    // the TASK-018 string_view lifetime contract.
     auto pmr_alloc = args.get_allocator();
     auto it = existing_it;
     if (it == args.end()) {
@@ -221,11 +257,12 @@ MHD_Result http_request_impl::build_request_args(void* cls, MHD_ValueKind kind,
             std::move(empty));
         it = inserted.first;
     }
-    // emplace_back into a pmr::vector<pmr::string>: use (ptr, size); the
-    // outer vector's allocator-propagating construct wires the inner
-    // pmr::string's allocator automatically. Passing the allocator
-    // ourselves leads to double-injection via uses-allocator construction.
-    it->second.emplace_back(value.data(), value.size());
+
+    // Allocate the destination pmr::string in the arena domain and
+    // copy the raw input bytes into it. The unescape transformation
+    // runs in-place on the arena-backed buffer.
+    auto& arg_value_ref = it->second.emplace_back(val_sv.data(), val_sv.size());
+    unescape_in_arena(arg_value_ref, aa->unescaper);
     return MHD_YES;
 }
 

src/detail/http_request_impl.cpp

@@ -57,6 +57,7 @@
 #include <vector>
 
 #include "httpserver/constants.hpp"
+#include "httpserver/detail/unescape_helpers.hpp"
 #include "httpserver/string_utilities.hpp"
 
 #if defined (__CYGWIN__)
@@ -302,49 +303,18 @@ std::string http_utils::sanitize_upload_filename(const std::string& filename) {
 // src/detail/ip_representation.cpp to keep this TU under the project
 // per-file LOC ceiling. See FILE_LOC_MAX in scripts/check-file-size.sh.
 
-static inline int hex_digit_value(char c) {
-    if (c >= '0' && c <= '9') return c - '0';
-    if (c >= 'a' && c <= 'f') return c - 'a' + 10;
-    if (c >= 'A' && c <= 'F') return c - 'A' + 10;
-    return -1;
-}
-
+// TASK-072 (code-simplifier-iter1-1/2): hex_digit_value and the core
+// unescape loop have been promoted to the shared internal header
+// src/httpserver/detail/unescape_helpers.hpp so that this TU and
+// src/detail/http_request_impl.cpp share one truth-source.
+// http_unescape is now a thin wrapper around unescape_buf_raw.
 size_t http_unescape(std::string* val) {
     if (val->empty()) return 0;
-
-    unsigned int rpos = 0;
-    unsigned int wpos = 0;
-
-    unsigned int size = val->size();
-
-    while (rpos < size && (*val)[rpos] != '\0') {
-        switch ((*val)[rpos]) {
-            case '+':
-                (*val)[wpos] = ' ';
-                wpos++;
-                rpos++;
-                break;
-            case '%':
-                if (size > rpos + 2) {
-                    int hi = hex_digit_value((*val)[rpos + 1]);
-                    int lo = hex_digit_value((*val)[rpos + 2]);
-                    if (hi >= 0 && lo >= 0) {
-                        (*val)[wpos] = static_cast<unsigned char>((hi << 4) | lo);
-                        wpos++;
-                        rpos += 3;
-                        break;
-                    }
-                }
-            // intentional fall through!
-            default:
-                (*val)[wpos] = (*val)[rpos];
-                wpos++;
-                rpos++;
-        }
-    }
-    (*val)[wpos] = '\0';  // add 0-terminator
-    val->resize(wpos);
-    return wpos;  // = strlen(val)
+    const std::size_t new_size =
+        httpserver::detail::unescape_buf_raw(val->data(), val->size());
+    (*val)[new_size] = '\0';  // add 0-terminator (std::string owns the byte)
+    val->resize(new_size);
+    return new_size;  // = strlen(val)
 }
 
 const std::string load_file(const std::string& filename) {

src/http_utils.cpp

@@ -0,0 +1,94 @@
+/*
+     This file is part of libhttpserver
+     Copyright (C) 2011-2026 Sebastiano Merlino
+
+     This library is free software; you can redistribute it and/or
+     modify it under the terms of the GNU Lesser General Public
+     License as published by the Free Software Foundation; either
+     version 2.1 of the License, or (at your option) any later version.
+
+     This library is distributed in the hope that it will be useful,
+     but WITHOUT ANY WARRANTY; without even the implied warranty of
+     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+     Lesser General Public License for more details.
+
+     You should have received a copy of the GNU Lesser General Public
+     License along with this library; if not, write to the Free Software
+     Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301
+     USA
+*/
+
+// Internal-only helpers for URL percent-decoding.
+//
+// Shared between src/http_utils.cpp (http_unescape) and
+// src/detail/http_request_impl.cpp (arena-routed unescape).
+// Not part of the public API; not installed.
+//
+// TASK-072: extracted from the two translation units that previously
+// each maintained a private copy.  Having one source of truth ensures
+// that bug-fixes and RFC-3986 edge-case corrections propagate uniformly.
+// (code-simplifier-iter1-1, code-simplifier-iter1-2)
+
+#pragma once
+
+#include <cstddef>
+
+namespace httpserver {
+namespace detail {
+
+// Map a single hex character to its integer value [0, 15].
+// Returns -1 for any character that is not a valid hex digit.
+//
+// constexpr so callers in constant-expression contexts can fold the
+// table at compile time; trivially inlined at -O1 and above.
+[[nodiscard]] constexpr int hex_digit_value(char c) noexcept {
+    if (c >= '0' && c <= '9') return c - '0';
+    if (c >= 'a' && c <= 'f') return c - 'a' + 10;
+    if (c >= 'A' && c <= 'F') return c - 'A' + 10;
+    return -1;
+}
+
+// Percent-decode and '+'-to-space unescape a raw byte buffer in-place.
+//
+// Reads from [data, data+size), writes the decoded bytes back into the
+// same buffer starting at data[0], and returns the new (decoded) length.
+// The output length is always <= the input length because %HH sequences
+// (3 bytes) decode to 1 byte and '+' stays 1 byte.
+//
+// The loop stops at a '\0' byte or when rpos reaches size, whichever
+// comes first, so embedded null bytes in the input are treated as
+// terminators (matching the behaviour of the original http_unescape).
+//
+// The caller is responsible for adding any terminating '\0' after the
+// returned length if the buffer must be C-string-compatible.
+inline std::size_t unescape_buf_raw(char* data, std::size_t size) noexcept {
+    if (size == 0) return 0;
+    std::size_t rpos = 0;
+    std::size_t wpos = 0;
+    while (rpos < size && data[rpos] != '\0') {
+        switch (data[rpos]) {
+            case '+':
+                data[wpos++] = ' ';
+                ++rpos;
+                break;
+            case '%':
+                if (size > rpos + 2) {
+                    const int hi = hex_digit_value(data[rpos + 1]);
+                    const int lo = hex_digit_value(data[rpos + 2]);
+                    if (hi >= 0 && lo >= 0) {
+                        data[wpos++] =
+                            static_cast<char>((hi << 4) | lo);
+                        rpos += 3;
+                        break;
+                    }
+                }
+            // intentional fall through!
+            default:
+                data[wpos++] = data[rpos++];
+        }
+    }
+    return wpos;
+}
+
+}  // namespace detail
+}  // namespace httpserver