Merge TASK-058: hot-path allocation pass

Three perf cleanups deferred from the manual-validation sweep:
  step 1 — canonicalize_lookup_path returns string_view (zero-alloc
    fast path; scratch buffer for non-canonical inputs).
  step 2 — auth_skip_paths normalized at webserver construction,
    plus empty-list early-out (~620 ns -> ~3 ns per request in the
    production-typical case).  Bonus: non-canonical skip-list
    entries now match canonical request paths.
  step 3 — lazy Allow-header cache on http_resource, invalidated
    implicitly via mask snapshot; std::shared_mutex so the warm
    path scales across worker threads.

Plus a bench_warm_path harness (step 0) capturing before/after
deltas for the three measured spots, and a TASK-058 validation
iter1 pass folding in security-reviewer-iter1 #1/#2/#3 and
performance-reviewer-iter1 #1.

Author: etr

specs/tasks/v2-deferred-backlog-plan.md

@@ -336,6 +336,7 @@ aggregation pipeline.
 **Milestone:** post-v2.0 (polish; not a release blocker)
 **Component:** `webserver_dispatch` / `webserver_request`
 **Estimate:** L (broken into ~4 sub-items)
+**Status:** Done
 
 **Goal:**
 Three perf items deferred during the manual-validation sweep all share
@@ -344,21 +345,50 @@ the same shape — a `std::string` is rebuilt on every request when a
 release-blocking but together they're worth 5–15% on the warm path.
 
 **Action Items:**
-- [ ] **canonicalize_lookup_path → string_view return.** Refactor
+- [x] **canonicalize_lookup_path → string_view return.** Refactor
   `src/detail/webserver_dispatch.cpp::canonicalize_lookup_path` to
   return a `string_view` into a caller-owned scratch buffer (or the
   request arena) instead of allocating a `std::string` on every call.
-- [ ] **normalize_path at registration time.** Move the
+  Done as step 1 (commit `e2f730d`): canonical-input fast path returns
+  the input view unchanged (zero allocation); non-canonical inputs
+  write into a caller-owned scratch `std::string`. Direct unit pin in
+  `test/unit/route_lookup_canonicalize_test.cpp`.
+- [x] **normalize_path at registration time.** Move the
   `normalize_path` call from `webserver_request.cpp::should_skip_auth`
   (where it runs per request) to the registration site, storing the
   normalized form in `route_entry`. Update the lookup to use the
   pre-normalized form.
-- [ ] **serialize_allow_methods caching.** `webserver_dispatch.cpp:363`
+  Done as step 2 (commit `51b5a37`). Interpretive deviation noted in
+  the commit message: the brief's `route_entry` framing doesn't map
+  onto `should_skip_auth`, which compares the request path against a
+  webserver-level `auth_skip_paths` list (not a per-route attribute);
+  the optimisation moves to the data's actual home —
+  `auth_skip_paths_normalized_` populated once at webserver
+  construction — plus an empty-list early-out for the production-
+  typical case. The empty-list early-out drops the per-request cost
+  from ~620 ns to ~3 ns. Bug-fix bonus: non-canonical skip-list
+  entries (`"/public/"`, `"/a/../b"`) now match canonical request paths.
+  Pinned in `test/unit/auth_skip_normalize_test.cpp`.
+- [x] **serialize_allow_methods caching.** `webserver_dispatch.cpp`
   rebuilds the Allow header value on every 405 response. Cache the
   serialized string on `route_entry` and invalidate on
   add/remove-method.
-- [ ] Add `test/bench_warm_path.cpp` measuring before/after per-request
+  Done as step 3 (commit `b22b0dd`). Interpretive deviation noted in
+  the commit message: the brief's `route_entry` framing would go stale
+  on `set_allowing` because `route_entry::methods` is the *registered*
+  mask, not the resource's runtime mask; the cache attaches to
+  `http_resource` instead (the same object that owns the mask) and
+  invalidates implicitly by comparing the live mask against a snapshot
+  at cache-fill time. Per-resource `std::mutex`. Pinned in
+  `test/unit/http_resource_allow_cache_test.cpp` (correctness +
+  identity / cache-hit-returns-same-buffer pin).
+- [x] Add `test/bench_warm_path.cpp` measuring before/after per-request
   cost so the next reviewer can verify the gain.
+  Done as step 0 (commit `18693a5`). Wired into `bench_targets` in
+  `test/Makefile.am` (not part of `make check`). Four measurements:
+  `canonicalize`, `should_skip_auth` (non-empty + empty), and
+  `serialize_allow_405`. Baseline numbers captured in the step-0
+  commit body; step-3 commit body holds the after-each-step deltas.
 
 **Dependencies:**
 - Blocked by: TASK-053 (must be the only caller of `canonicalize_lookup_path`)

src/detail/webserver_dispatch.cpp

@@ -123,25 +123,57 @@ void webserver_impl::invalidate_route_cache() {
 // would never share an entry. Matches the v1 dispatch path, which
 // constructs a non-registration http_endpoint at lookup time and so
 // gets the same normalization for free.
-static std::string canonicalize_lookup_path(const std::string& path) {
+//
+// TASK-058 step 1: return a string_view so the happy path (input
+// already canonical) allocates zero heap memory.  On the rewrite
+// path the canonicalised form is written into the caller-owned
+// @p scratch buffer and a view into that buffer is returned.
+//
+// LIFETIME: the returned view is valid for the duration of the
+// call chain only; it points at either the immutable "/" literal,
+// the caller's @p path argument, or the caller's @p scratch buffer.
+// Any caller storing the view must copy it into an owning string
+// first.  The cache layer (cache_key constructed below) already
+// copies via std::string, so the contract is naturally respected.
+static std::string_view canonicalize_lookup_path(
+        std::string_view path, std::string& scratch) {
     if (path.empty()) {
-        return "/";
+        // Immutable canonical root -- no scratch usage.
+        return std::string_view{"/", 1};
+    }
+    const bool has_leading_slash = (path.front() == '/');
+    const bool has_trailing_slash = (path.size() > 1 && path.back() == '/');
+    if (has_leading_slash && !has_trailing_slash) {
+        // Already canonical: return the caller's view directly.
+        return path;
     }
-    std::string out = path;
-    if (out.front() != '/') {
-        out.insert(out.begin(), '/');
+    // Rewrite path: write the canonicalised form into the caller's
+    // scratch buffer and return a view into it.
+    scratch.clear();
+    scratch.reserve(path.size() + (has_leading_slash ? 0 : 1));
+    if (!has_leading_slash) {
+        scratch.push_back('/');
     }
-    if (out.size() > 1 && out.back() == '/') {
-        out.pop_back();
+    if (has_trailing_slash) {
+        scratch.append(path.data(), path.size() - 1);
+    } else {
+        scratch.append(path.data(), path.size());
     }
-    return out;
+    return std::string_view{scratch};
 }
 
 webserver_impl::lookup_result
 webserver_impl::lookup_v2(http_method method, const std::string& path) {
     lookup_result result;
 
-    std::string lookup_path = canonicalize_lookup_path(path);
+    // TASK-058 step 1: canonicalize_lookup_path returns a string_view
+    // into either @p path (already-canonical happy path -- no
+    // allocation), the immutable "/" literal (empty-input case), or
+    // @p canonicalize_scratch (rewrite case -- single allocation,
+    // bounded by the input size).
+    std::string canonicalize_scratch;
+    std::string_view lookup_path =
+        canonicalize_lookup_path(path, canonicalize_scratch);
 
     // Step 1: cache. Cache under the canonical key so /foo and /foo/
     // share an entry. Use find_by_view to avoid copying lookup_path
@@ -155,10 +187,11 @@ webserver_impl::lookup_v2(http_method method, const std::string& path) {
         result.captured_params = std::move(cached.captured_params);
         return result;
     }
-    // Construct key by moving lookup_path into the cache_key, avoiding a
-    // second heap allocation. All subsequent tier lookups use key.path,
-    // which is the same std::string now owned by the key.
-    cache_key key{method, std::move(lookup_path)};
+    // Construct key by copying lookup_path into the cache_key.  On the
+    // miss path we pay one std::string construction; the warm-cache
+    // path never reaches this line.  All subsequent tier lookups use
+    // key.path, which is the std::string now owned by the key.
+    cache_key key{method, std::string(lookup_path)};
 
     // Step 2: walk the tiers under a shared lock.
     {
@@ -420,9 +453,15 @@ void webserver_impl::dispatch_resource_handler(detail::modded_request* mr,
             }
             return;
         }
-        // Method not allowed: serialize the allow-mask into a header.
+        // Method not allowed: emit the Allow header.  TASK-058 step 3:
+        // read the resource's lazily-cached Allow header value instead
+        // of rebuilding the string from the mask on every 405.  The
+        // cache lives on http_resource (the same object that owns the
+        // mask), regenerates implicitly when the mask differs from the
+        // last-cached snapshot, and is thread-safe under a per-resource
+        // mutex held only across the cache-fill path.
         mr->response.emplace(method_not_allowed_page(mr));
-        std::string header_value = serialize_allow_methods(hrm->get_allowed_methods());
+        const std::string& header_value = hrm->get_allow_header();
         if (!header_value.empty()) {
             mr->response->with_header(http_utils::http_header_allow, header_value);
         }

src/detail/webserver_request.cpp

@@ -67,6 +67,7 @@
 #include "httpserver/detail/http_endpoint.hpp"
 #include "httpserver/detail/lambda_resource.hpp"
 #include "httpserver/detail/modded_request.hpp"
+#include "httpserver/detail/path_normalize.hpp"
 #include "httpserver/detail/resource_hook_table.hpp"
 #include "httpserver/http_request.hpp"
 #include "httpserver/http_resource.hpp"
@@ -136,16 +137,95 @@ std::string normalize_path(const std::string& path) {
 
 }  // namespace
 
+// TASK-058 step 2: pre-normalize each auth_skip_paths entry once at
+// webserver construction time.  Entries ending in "/*" keep their
+// wildcard suffix; the prefix before the wildcard is normalized.
+// Callers (webserver::webserver) pass the raw config-bag list and
+// store the result on the webserver instance as a sibling to the
+// original `auth_skip_paths` list.  Pre-TASK-058 the skip list was
+// matched verbatim against a normalized request path, so non-
+// canonical inputs (e.g. "/public/", "/a/../b") silently never
+// matched -- the on-the-wire bug this normalization closes.
+//
+// security-reviewer-iter1-3: entries containing '%' are rejected with
+// std::invalid_argument.  Skip-path entries must be provided in
+// decoded form (the same form as the request path after MHD's
+// unescaper runs).  A '%'-encoded entry would never match a decoded
+// request path and would silently bypass auth for no route -- a
+// misconfiguration hazard caught early here.
+std::vector<std::string> normalize_auth_skip_paths(
+        const std::vector<std::string>& raw) {
+    std::vector<std::string> out;
+    out.reserve(raw.size());
+    for (const auto& entry : raw) {
+        // Reject percent-encoded entries: skip-path entries must be
+        // provided in decoded form.  A '%' in the entry indicates a
+        // URL-encoded sequence that would never match the decoded
+        // request path produced by MHD's unescaper.
+        if (entry.find('%') != std::string::npos) {
+            throw std::invalid_argument(
+                "auth_skip_paths entry contains a percent-encoded "
+                "sequence ('" + entry + "'). "
+                "Skip-path entries must be provided in decoded form "
+                "(e.g. '/public/test', not '/public%2Ftest').");
+        }
+        // Wildcard suffix: strip the trailing "/*", normalize the
+        // prefix, then re-append "/*".  The special case "/*" (size
+        // == 2) means "match every path" and is stored as-is so
+        // should_skip_auth can recognise it with the >= 2 guard.
+        if (entry.size() >= 2 && entry.back() == '*' &&
+            entry[entry.size() - 2] == '/') {
+            if (entry.size() == 2) {
+                // "/*" -- global wildcard: matches every path.
+                out.push_back("/*");
+            } else {
+                std::string prefix = entry.substr(0, entry.size() - 2);
+                std::string normalized_prefix = normalize_path(prefix);
+                if (normalized_prefix == "/") {
+                    // Prefix collapsed to root -- treat as "/*".
+                    out.push_back("/*");
+                } else {
+                    out.push_back(normalized_prefix + "/*");
+                }
+            }
+            continue;
+        }
+        out.push_back(normalize_path(entry));
+    }
+    return out;
+}
+
 bool webserver_impl::should_skip_auth(const std::string& path) const {
+    // TASK-058 step 2: empty-list early-out.  Servers with no
+    // auth_skip_paths configured pay zero normalization cost.  This
+    // is the production-typical case for any server whose auth
+    // surface either covers every route or has no auth_handler at
+    // all.
+    if (parent->auth_skip_paths_normalized.empty()) {
+        return false;
+    }
+
+    // TASK-058 step 2: compare against the pre-normalized list (built
+    // once at construction time) instead of re-normalizing skip-list
+    // entries on every request.  The per-request normalize_path call
+    // on @p path remains -- the inbound URL is per-request data and
+    // cannot be pre-normalized.
     std::string normalized = normalize_path(path);
 
-    for (const auto& skip_path : parent->auth_skip_paths) {
+    for (const auto& skip_path : parent->auth_skip_paths_normalized) {
         if (skip_path == normalized) return true;
-        // Support wildcard suffix (e.g., "/public/*")
-        if (skip_path.size() > 2 && skip_path.back() == '*' &&
+        // Support wildcard suffix (e.g., "/public/*").
+        // security-reviewer-iter1-1: use >= 2 (not > 2) so the global
+        // wildcard "/*" (size == 2) is handled.  When skip_path is "/*"
+        // the prefix is "/" and every normalized path starts with "/",
+        // so we return true immediately for any request.
+        if (skip_path.size() >= 2 && skip_path.back() == '*' &&
             skip_path[skip_path.size() - 2] == '/') {
-            std::string prefix = skip_path.substr(0, skip_path.size() - 1);
-            if (normalized.compare(0, prefix.size(), prefix) == 0) return true;
+            std::string_view prefix(skip_path.data(), skip_path.size() - 1);
+            if (normalized.compare(0, prefix.size(), prefix.data(),
+                                   prefix.size()) == 0) {
+                return true;
+            }
         }
     }
     return false;

src/http_resource.cpp

@@ -28,10 +28,12 @@
 #include <atomic>
 #include <functional>
 #include <memory>
+#include <shared_mutex>
 #include <stdexcept>
 #include <string>
 #include <utility>
 
+#include "httpserver/detail/method_utils.hpp"
 #include "httpserver/detail/resource_hook_table.hpp"
 #include "httpserver/hook_action.hpp"
 #include "httpserver/hook_context.hpp"
@@ -97,6 +99,55 @@ ensure_table(std::shared_ptr<detail::resource_hook_table>& slot) {
 
 }  // namespace
 
+// ---- copy / move special members ----------------------------------------
+//
+// TASK-058 step 3 added std::shared_mutex cached_allow_mutex_, which
+// has no copy or move.  The defaulted special members on the public
+// class declaration would therefore be implicitly deleted.  Implement
+// them here by-hand, skipping the mutex.  The copy / move target starts
+// with a fresh, default-constructed mutex and an invalidated Allow-
+// header cache (cached_allow_valid_ = false), which the next call to
+// get_allow_header() repopulates lazily.
+
+http_resource::http_resource(const http_resource& b) noexcept
+    : methods_allowed_(b.methods_allowed_),
+      hook_table_(b.hook_table_) {
+    // cached_allow_mutex_ default-constructs.
+    // cached_allow_header_ / cached_allow_mask_ default-construct.
+    // cached_allow_valid_ stays false (member default).
+}
+
+http_resource::http_resource(http_resource&& b) noexcept
+    : methods_allowed_(b.methods_allowed_),
+      hook_table_(std::move(b.hook_table_)) {
+    // Same rationale as the copy constructor: cache state is per-
+    // instance and is not transferred.  std::shared_mutex has no move.
+}
+
+http_resource& http_resource::operator=(const http_resource& b) noexcept {
+    if (this != &b) {
+        hook_table_ = b.hook_table_;
+        methods_allowed_ = b.methods_allowed_;
+        // Invalidate the local cache; do NOT touch the mutex (still
+        // owned by *this).
+        std::unique_lock<std::shared_mutex> lock(cached_allow_mutex_);
+        cached_allow_valid_ = false;
+        cached_allow_header_.clear();
+    }
+    return *this;
+}
+
+http_resource& http_resource::operator=(http_resource&& b) noexcept {
+    if (this != &b) {
+        hook_table_ = std::move(b.hook_table_);
+        methods_allowed_ = b.methods_allowed_;
+        std::unique_lock<std::shared_mutex> lock(cached_allow_mutex_);
+        cached_allow_valid_ = false;
+        cached_allow_header_.clear();
+    }
+    return *this;
+}
+
 // ---- add_hook overloads -------------------------------------------------
 
 hook_handle http_resource::add_hook(hook_phase phase,
@@ -149,4 +200,53 @@ hook_handle http_resource::add_hook(hook_phase phase,
                        hook_phase::request_completed, id};
 }
 
+// ---- get_allow_header ---------------------------------------------------
+//
+// TASK-058 step 3: lazy Allow-header cache.  See the header-side
+// declaration for the contract.  Implementation:
+//
+//   Warm path (cache hit, the common case after the first 405):
+//     1. Take a shared lock.
+//     2. Read methods_allowed_ INSIDE the lock (eliminates the TOCTOU
+//        data race flagged in security-reviewer-iter1-2).
+//     3. Compare with the cached snapshot; if they match, return the
+//        cached string by reference while still holding the shared lock.
+//
+//   Cold / stale path (cache miss or mask changed):
+//     1. Release the shared lock.
+//     2. Take an exclusive (unique) lock.
+//     3. Re-check (double-checked locking) because another thread may
+//        have filled the cache between the two lock acquisitions.
+//     4. Rebuild via detail::format_allow_header and update the snapshot.
+//     5. Return the cached string by reference.
+//
+// Using std::shared_mutex allows N concurrent warm-path readers without
+// serialisation; only the cold fill path serialises
+// (performance-reviewer-iter1-1).
+//
+// The returned reference is stable until the next mask mutation; the
+// caller (dispatch_resource_handler) consumes it synchronously within
+// the current dispatch, so the reference outlives use.
+const std::string& http_resource::get_allow_header() const {
+    // Warm path: shared (read) lock.
+    {
+        std::shared_lock<std::shared_mutex> rlock(cached_allow_mutex_);
+        const method_set live = methods_allowed_;  // read inside lock
+        if (cached_allow_valid_ && cached_allow_mask_ == live) {
+            return cached_allow_header_;
+        }
+    }
+    // Cold / stale path: exclusive (write) lock.
+    std::unique_lock<std::shared_mutex> wlock(cached_allow_mutex_);
+    // Double-checked: another thread may have filled the cache while we
+    // waited for the exclusive lock.
+    const method_set live = methods_allowed_;  // re-read inside write lock
+    if (!cached_allow_valid_ || cached_allow_mask_ != live) {
+        cached_allow_header_ = detail::format_allow_header(live);
+        cached_allow_mask_ = live;
+        cached_allow_valid_ = true;
+    }
+    return cached_allow_header_;
+}
+
 }  // namespace httpserver