Merge TASK-015 review cleanup (18/64 fixed, 46 deferred)

Author: etr

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

@@ -2,7 +2,7 @@
 
 **Responsibility:** Carry per-request inputs from MHD's worker thread to the user handler. Lazily-cache derived data (path pieces, parsed args, basic-auth credentials, client cert fields).
 
-**Implementation:** PIMPL via `std::unique_ptr<http_request_impl>`. The impl is **arena-allocated** from a `std::pmr::monotonic_buffer_resource` that lives on the connection (one arena per MHD connection, reset between requests on the same keep-alive connection). The arena also backs the impl's owned strings and lazy-cache containers where practical, eliminating per-request `malloc` on the hot path.
+**Implementation:** PIMPL via `std::unique_ptr<http_request_impl>` (with a custom deleter that supports both heap and arena lifetimes). The impl is **arena-allocated** from a `std::pmr::monotonic_buffer_resource` that lives on the connection (one arena per MHD connection, reset between requests on the same keep-alive connection). The arena also backs the impl's owned strings and lazy-cache containers where practical, eliminating per-request `malloc` on the hot path. *[Historical note: the PIMPL boundary was introduced in TASK-015 using `std::make_unique` as a temporary heap allocation; the per-connection arena plumbing was wired in TASK-016, completing the DR-003b Option 2 decision.]*
 
 **Interfaces:**
 - Exposes (from PRD §3.6):

specs/architecture/11-decisions/DR-003b.md

@@ -21,4 +21,8 @@
 - `webserver` constructor takes the arena allocator out of band (request constructor receives it implicitly via the dispatch path; not a public API surface).
 - `std::pmr::polymorphic_allocator` plumbed through `webserver_impl` → connection state → request ctor.
 
+**Implementation phasing:**
+- *TASK-015* introduced the PIMPL boundary with `std::make_unique<http_request_impl>()` (plain heap allocation) as an incremental step to split the public header from the backend-coupled impl. This was an intentional temporary deviation from the full Option 2 design.
+- *TASK-016* wired the per-connection `std::pmr::monotonic_buffer_resource` arena through `webserver_impl::connection_notify` and the `pick_resource()` helper in `http_request.cpp`, completing the DR-003b Option 2 decision. The `http_request_impl` constructor and all its container members are PMR-aware as of TASK-016.
+
 ---

specs/unworked_review_issues/2026-05-04_013108_task-013.md

@@ -20,9 +20,10 @@
    The unauthorized() factory drops the v1 digest-auth nonce/opaque state-machine (previously using MHD_queue_auth_required_response3) in favour of a static WWW-Authenticate challenge. This is a meaningful behavioural regression for callers relying on full MHD digest-auth. The commit message and the http_response.hpp doc-comment (line 183-188) acknowledge the trade-off, but no Decision Record captures it. DR-005 covers body representation; no DR covers the auth-flow simplification. The architecture doc §4.3 lists unauthorized(scheme, realm, ...) as the factory without mentioning the state-machine loss.
    *Recommendation:* Create a new DR (e.g. DR-012) documenting: (a) the decision to replace MHD_queue_auth_required_response3 with a static WWW-Authenticate challenge, (b) the rationale (removes dependency on connection-time state / MHD internal nonce tracking from the response value type), and (c) the consequence that callers wanting full RFC 7616 digest auth must reach MHD APIs directly. Reference PRD-RSP-REQ-005. This brings the arch record into sync with the implementation without changing any code.
 
-4. [ ] **architecture-alignment-checker** | `src/httpserver/http_request.hpp:28` | interface-contract
+4. [x] **architecture-alignment-checker** | `src/httpserver/http_request.hpp:28` | interface-contract
    http_request.hpp still includes <microhttpd.h> directly (line 28), leaking MHD types into the public umbrella header via httpserver.hpp. Like the webserver.hpp case this predates TASK-013 (M3 / TASK-015 is assigned to fix it). TASK-013's stated acceptance check was narrowly scoped to http_response.hpp, and that file is now clean — no struct MHD_Connection or struct MHD_Response forward declarations remain in http_response.hpp (confirmed). No regression was introduced by this commit.
    *Recommendation:* Track alongside finding #2 as a pre-M3 accepted deviation. Verify that the M3 PIMPL tasks (TASK-014, TASK-015) clear both headers.
+   *Status:* fixed — TASK-015 removed `#include <microhttpd.h>` from http_request.hpp; confirmed absent (grep returns nothing for the include in the public header, which now carries only a comment at lines 28-38 explaining the deliberate absence). Closed by TASK-015 review cleanup batch (2026-05-27).
 
 5. [ ] **architecture-alignment-checker** | `src/httpserver/webserver.hpp:28` | component-boundary
    webserver.hpp includes <microhttpd.h>, <pthread.h>, and <sys/socket.h> in the public header, which conflicts with the documented architecture goal in §4.1: 'Public header <httpserver/webserver.hpp> includes only <httpserver/create_webserver.hpp> and standard library, never <microhttpd.h> or <pthread.h>.' This pre-dates TASK-013 and is tracked as deferred work (TASK-014 PIMPL split, M3 milestone per specs/tasks/_index.md), but the constraint remains unmet in the post-TASK-013 state.

specs/unworked_review_issues/2026-05-04_211034_task-015.md

@@ -43,17 +43,25 @@ namespace httpserver {
 
 namespace detail {
 
+// Map a MHD_ValueKind to the corresponding test-request local storage map.
+// Returns nullptr for kinds that have no local-storage counterpart (e.g.
+// MHD_GET_ARGUMENT_KIND). Called from both get_connection_value() and
+// ensure_headerlike_cache() to keep the kind→map dispatch in one place.
+// Adding a new kind (e.g. MHD_POSTDATA_KIND) only requires updating this
+// function. (code-simplifier-iter1 findings #3/#4 / major review items)
+const http::header_map* http_request_impl::local_map_for(MHD_ValueKind kind) const noexcept {
+    switch (kind) {
+        case MHD_HEADER_KIND: return &headers_local;
+        case MHD_FOOTER_KIND: return &footers_local;
+        case MHD_COOKIE_KIND: return &cookies_local;
+        default:              return nullptr;
+    }
+}
+
 std::string_view http_request_impl::get_connection_value(std::string_view key, MHD_ValueKind kind) const {
     // Test-request path: connection_ is null, fall back to local storage.
     if (connection_ == nullptr) {
-        const auto* map = [&]() -> const http::header_map* {
-            switch (kind) {
-                case MHD_HEADER_KIND: return &headers_local;
-                case MHD_FOOTER_KIND: return &footers_local;
-                case MHD_COOKIE_KIND: return &cookies_local;
-                default:             return nullptr;
-            }
-        }();
+        const auto* map = local_map_for(kind);
         if (map != nullptr) {
             // header_comparator is_transparent: find(string_view) works without
             // constructing a temporary std::string. (performance-reviewer-iter1-30)
@@ -83,34 +91,32 @@ MHD_Result http_request_impl::build_request_header(void* cls, MHD_ValueKind kind
 const http::header_view_map& http_request_impl::ensure_headerlike_cache(MHD_ValueKind kind) const {
     // Pick the cache slot and build-flag matching `kind`. We resolve them
     // up front so the cold (build) and warm (return) paths share a single
-    // reference without re-switching.
+    // reference without re-switching. local_map_for() provides the
+    // local-storage fallback pointer without duplicating the switch.
+    // (code-simplifier-iter1 findings #3/#4 / major review items)
     http::header_view_map* cache = nullptr;
     bool* built = nullptr;
-    const http::header_map* local_fallback = nullptr;
     switch (kind) {
         case MHD_HEADER_KIND:
             cache = &headers_cached_;
             built = &headers_cache_built_;
-            local_fallback = &headers_local;
             break;
         case MHD_FOOTER_KIND:
             cache = &footers_cached_;
             built = &footers_cache_built_;
-            local_fallback = &footers_local;
             break;
         case MHD_COOKIE_KIND:
             cache = &cookies_cached_;
             built = &cookies_cache_built_;
-            local_fallback = &cookies_local;
             break;
         default:
             // Unsupported kind: hand back the headers cache (kept empty)
             // as a safe fallback; the public API never reaches here.
             cache = &headers_cached_;
             built = &headers_cache_built_;
-            local_fallback = &headers_local;
             break;
     }
+    const http::header_map* local_fallback = local_map_for(kind);
 
     if (*built) {
         return *cache;
@@ -350,7 +356,8 @@ void http_request_impl::grow_last_arg(const std::string& key, const std::string&
 
 #ifdef HAVE_BAUTH
 void http_request_impl::fetch_user_pass() const {
-    // Test-request path: connection_ is null, credentials already set.
+    // Test-request path: connection_ is null, credentials already set
+    // directly by create_test_request; nothing to fetch.
     if (connection_ == nullptr) {
         return;
     }
@@ -363,6 +370,11 @@ void http_request_impl::fetch_user_pass() const {
         }
         MHD_free(info);
     }
+    // Mark as fetched so subsequent get_user()/get_pass() calls skip the
+    // MHD round-trip -- even when the request carries no auth header and
+    // the credential strings remain empty after this call.
+    // (code-simplifier-iter1 finding #6 / major review item)
+    user_pass_fetched = true;
 }
 #endif  // HAVE_BAUTH
 

src/detail/http_request_impl.cpp

@@ -309,12 +309,15 @@ std::string_view http_request::get_querystring() const noexcept {
 
 
 std::string_view http_request::get_requestor() const {
-    if (!impl_->requestor_ip.empty()) {
-        return impl_->requestor_ip;
-    }
-
-    // Test-request path: connection_ is null, requestor_ip already set.
-    if (impl_->connection_ == nullptr) {
+    // Single consolidated early-return covers both the cache-hit path
+    // (requestor_ip_cached == true after first call on a live connection)
+    // and the test-request path (connection_ == nullptr, requestor_ip was
+    // set directly by create_test_request). Using a dedicated boolean instead
+    // of checking requestor_ip.empty() is consistent with the args_populated /
+    // path_pieces_cached / user_pass_fetched pattern and is robust if the
+    // connection layer ever returns an empty IP string.
+    // (code-simplifier-iter1 findings #7 + #8/#9 / major+minor review items)
+    if (impl_->requestor_ip_cached || impl_->connection_ == nullptr) {
         return impl_->requestor_ip;
     }
 
@@ -326,6 +329,7 @@ std::string_view http_request::get_requestor() const {
 
     auto ip = http::get_ip_str(conninfo->client_addr);
     impl_->requestor_ip.assign(ip.data(), ip.size());
+    impl_->requestor_ip_cached = true;
     return impl_->requestor_ip;
 }
 

src/http_request.cpp

@@ -41,10 +41,18 @@ namespace httpserver {
 
 std::string_view http_request::get_user() const {
 #ifdef HAVE_BAUTH
-    if (!impl_->username.empty()) {
-        return impl_->username;
+    // Use user_pass_fetched instead of !username.empty() as the cache
+    // guard: an empty username string after a successful MHD call is a
+    // valid result (no auth header), but would cause a redundant re-fetch
+    // on every subsequent get_user() call without the boolean.
+    // (code-simplifier-iter1 finding #6 / major review item)
+    if (!impl_->user_pass_fetched) {
+        impl_->fetch_user_pass();
+        // fetch_user_pass() sets user_pass_fetched = true on the live path.
+        // On the test-request path (connection_ == nullptr) it returns early
+        // without setting the flag; set it here so future calls are skipped.
+        impl_->user_pass_fetched = true;
     }
-    impl_->fetch_user_pass();
     return impl_->username;
 #else
     // TASK-034 §7 sentinel: BAUTH disabled at build time -> empty view.
@@ -54,10 +62,11 @@ std::string_view http_request::get_user() const {
 
 std::string_view http_request::get_pass() const {
 #ifdef HAVE_BAUTH
-    if (!impl_->password.empty()) {
-        return impl_->password;
+    // Mirror the get_user() boolean guard (code-simplifier-iter1 #6).
+    if (!impl_->user_pass_fetched) {
+        impl_->fetch_user_pass();
+        impl_->user_pass_fetched = true;
     }
-    impl_->fetch_user_pass();
     return impl_->password;
 #else
     return std::string_view{};

src/http_request_auth.cpp

@@ -131,9 +131,14 @@ class http_request_impl {
 
     http_request_impl(const http_request_impl&) = delete;
     http_request_impl& operator=(const http_request_impl&) = delete;
-    // Moves are left implicitly defined (and unused -- the impl is held
-    // through unique_ptr so http_request's defaulted moves operate on
-    // the unique_ptr, not on the impl directly).
+    // Move operations: explicitly defaulted to document intent.
+    // The impl is held through a custom-deleter unique_ptr, so http_request's
+    // own move ctor/assign operate on the unique_ptr, never on the impl
+    // directly. These defaults are unused in practice but explicit = default
+    // is clearer than relying on the implicit definition.
+    // (code-simplifier-iter1 finding #33 / minor review item)
+    http_request_impl(http_request_impl&&) = default;
+    http_request_impl& operator=(http_request_impl&&) = default;
 
     // --- per-request backend handles ---
     MHD_Connection* connection_ = nullptr;
@@ -181,6 +186,23 @@ class http_request_impl {
     mutable std::pmr::vector<std::pmr::string> path_pieces;
     mutable bool args_populated = false;
     mutable bool path_pieces_cached = false;
+#ifdef HAVE_BAUTH
+    // Guard for fetch_user_pass(): once the MHD round-trip has been made
+    // (whether or not the request carries a Basic-Auth header), further
+    // calls to get_user()/get_pass() skip the MHD call entirely.
+    // Using a dedicated boolean (rather than checking username.empty())
+    // matches the args_populated / path_pieces_cached pattern and avoids
+    // a redundant MHD call on requests with no auth credentials where the
+    // credential strings remain empty after the first fetch.
+    // (code-simplifier finding #6 / major review item)
+    mutable bool user_pass_fetched = false;
+#endif  // HAVE_BAUTH
+    // Cache guard for get_requestor(). Using a dedicated boolean (rather
+    // than checking requestor_ip.empty()) is consistent with the boolean-
+    // flag pattern used by args_populated and path_pieces_cached, and is
+    // robust if the connection layer ever returns an empty IP string.
+    // (code-simplifier finding #7 / minor review item)
+    mutable bool requestor_ip_cached = false;
 
     // TASK-017: per-request caches for the six container getters. These
     // are typed in the public-API container types (default-allocator) so
@@ -233,6 +255,13 @@ class http_request_impl {
 #endif  // HAVE_GNUTLS
 
     // --- helpers (moved out of http_request public header) ---
+
+    // Map MHD_ValueKind to the corresponding test-request local-storage map
+    // pointer (nullptr for kinds with no local counterpart). Centralises the
+    // kind→map dispatch so get_connection_value() and ensure_headerlike_cache()
+    // share one switch body. (code-simplifier-iter1 findings #3/#4)
+    const http::header_map* local_map_for(MHD_ValueKind kind) const noexcept;
+
     std::string_view get_connection_value(std::string_view key, MHD_ValueKind kind) const;
     // TASK-017: ensures the cache for `kind` (HEADER / FOOTER / COOKIE) is
     // populated and returns a const reference to it. First call fills the