v2.0: Modernization (M1-M6, 44 tasks)#374
Draft
etr wants to merge 493 commits into
Draft
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #374 +/- ##
==========================================
+ Coverage 68.03% 68.72% +0.68%
==========================================
Files 34 64 +30
Lines 1730 4057 +2327
Branches 697 1489 +792
==========================================
+ Hits 1177 2788 +1611
- Misses 80 357 +277
- Partials 473 912 +439
... and 21 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
etr
added a commit
that referenced
this pull request
May 7, 2026
…rueFalse, exclude specs/ Codacy was reporting 2018 new issues on the v2.0 PR (#374). Resolve as follows: * Add .codacy.yaml excluding specs/** — the product spec, architecture notes, task records, and review notes are internal groundwork artifacts, not user-facing docs, and should not be subject to README markdownlint rules. Removes 2003 markdownlint findings. * src/webserver.cpp:499 — drop the redundant `blocking &&` from the wait loop condition. `blocking` is a function parameter never reassigned inside the loop body, so the conjunct was tautological (cppcheck knownConditionTrueFalse). * src/webserver.cpp:946 — replace the C-style `(struct detail::modded_request*)` cast on the MHD `cls` void* with `static_cast<detail::modded_request*>` (cppcheck cstyleCast). Mirrors the existing static_cast usage elsewhere in the file. * detail/webserver_impl.hpp, detail/http_request_impl.hpp, iovec_entry.hpp — add `// cppcheck-suppress-file unusedStructMember` with a one-line rationale comment. Every flagged member is in fact heavily used from the corresponding .cpp translation unit (registered_resources*, route_cache_*, bans, allowances, files_, path_pieces_public_, iovec_entry::base/len, etc.); cppcheck analyses each TU in isolation and cannot see those uses, so the warning is a known pimpl/POD false positive. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
etr
added a commit
that referenced
this pull request
May 7, 2026
… clash Two unrelated CI regressions on PR #374, both falling out of TASK-020: 1. Lint job (gcc-14, ubuntu): cpplint flagged src/http_utils.cpp:30 with build/include_order, because the matching public header ("httpserver/http_utils.hpp") came AFTER a non-matching project header ("httpserver/constants.hpp"), and <microhttpd.h> (a C system header in cpplint's view) followed both. cpplint's expected order is: matching header, C system, C++ system, other. Reorder so the matching header comes first and the project headers ("constants.hpp" / "string_utilities.hpp") move to the bottom of the include block. 2. Windows MSYS2 build: src/httpserver/http_utils.hpp failed with error: expected identifier before numeric constant at the line `ERROR = 0,` inside the digest_auth_result enum. <wingdi.h> (pulled in via <windows.h> via <winsock2.h> via <microhttpd.h> on MinGW) unconditionally `#define`s ERROR to 0, and the preprocessor expands macros inside scoped-enum bodies just like anywhere else. Pre-TASK-020 the enum was inside `#ifdef HAVE_DAUTH`, so MSYS2 builds without digest auth never compiled it; PRD-FLG-REQ-001 then made the enum unconditional and exposed the latent collision. v2.0 is unreleased, so renaming is safe: ERROR -> GENERIC_ERROR (matches MHD_DAUTH_ERROR's "general error" docs). Static-assert pin in src/http_utils.cpp updated to match. Verified locally: - python3 -m cpplint on both touched files: exit 0. - `make check` on macOS: 32/32 PASS, all check-hygiene / check-headers gates PASS. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
etr
added a commit
that referenced
this pull request
May 11, 2026
Codacy's "26 new issues (0 max.)" gate was failing on PR #374. Two classes of finding, addressed at root: - 21 markdownlint findings on test/REGRESSION.md (MD013 line-length, MD040 fenced-code language, MD043 heading structure). REGRESSION.md is an internal test-gate document (the v2.0 routing parity gate), conceptually peer to the already-excluded specs/ artifacts and not in the user-facing README/ChangeLog/CONTRIBUTING category. Extend .codacy.yaml exclude_paths with `test/**/*.md`. - 5 cppcheck findings that are all single-TU false positives: * iovec_entry.hpp: `cppcheck-suppress-file unusedStructMember` was not at the top of the file (preprocessorErrorDirective), so the file-level suppression was ignored and `base`/`len` were both flagged unused. Replaced with per-member inline suppressions. * route_cache.hpp: `cache_value::captured_params` is read in src/webserver.cpp at the cache-hit replay site; cppcheck does not follow the cross-TU read. Inline-suppress. * header_hygiene_test.cpp: cppcheck statically assumes none of the forbidden-header guard macros are defined and reports `leaks > 0` as always-false; the comparison is load-bearing at runtime under any actual leak. Inline-suppress. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
etr
added a commit
that referenced
this pull request
May 20, 2026
Three CI failures on feature/v2.0 PR #374 run 26183259463: 1. cpplint: examples/hello_world.cpp was missing the copyright line. Added single-line copyright header (the file is the deliberately minimal lambda-form example, so the full LGPL block would defeat its purpose). 2. tsan ws_start_stop: webserver::stop() and is_running() read impl_->running with no lock while start() writes it from the blocking-server thread. Made the field std::atomic<bool> — fixes the genuine race without changing the mutex/cond_var discipline that gates the blocking wait. 3. tsan route_table_concurrency + threadsafety_stress: libstdc++'s std::ctype<char>::narrow lazily fills a 256-byte cache; the guard flag is not atomic so concurrent std::regex compiles inside http_endpoint::http_endpoint look like a race even though every initialiser computes the same bytes. Added test/tsan.supp scoped to that one libstdc++ symbol pair, plumbed via TSAN_OPTIONS only on the tsan matrix lane, and shipped via test/Makefile.am EXTRA_DIST. Libhttpserver-internal races stay fatal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
etr
added a commit
that referenced
this pull request
May 21, 2026
Planning-only commit. No code yet; subsequent task-branch PRs implement TASK-045..052 in order against feature/v2.0. Adds a multi-subscriber lifecycle hook system to v2.0, replacing v1's patchwork of single-slot callbacks (log_access, not_found_handler, method_not_allowed_handler, internal_error_handler, auth_handler) with one uniform webserver::add_hook(phase, callable) surface plus a per-route http_resource::add_hook(...) variant. Existing v1 setters survive as documented aliases (PRD-HOOK-REQ-009). Eleven phases spanning the connection -> request -> routing -> handler -> response -> cleanup lifecycle: connection_opened, accept_decision, request_received, body_chunk, route_resolved, before_handler, handler_exception, after_handler, response_sent, request_completed, connection_closed. Short-circuit allowed at four pre-handler phases (request_received, body_chunk, before_handler, handler_exception) and at the after_handler post-handler phase. Throwing hooks route through DR-9 §5.2. Closes (once TASK-046, 047, 050 land): #332 banned-IP log entry (accept_decision hook) #281 response-aware access log (response_sent context) #69 Common Log Format w/ time-taken (response_sent context) #273 early 413 on oversize body (request_received short-circuit) Partially addresses #272 (body_chunk observation; the buffer-steal half remains a v2.1 candidate needing a streaming-body API). Files added: specs/architecture/11-decisions/DR-012.md specs/architecture/04-components/hooks.md (§4.10) specs/tasks/M5-routing-lifecycle/TASK-045.md .. TASK-052.md Files updated: specs/product_specs.md - new §3.8 with PRD-HOOK-REQ-001..009 - §4 traceability line for API-HOOK specs/architecture/05-cross-cutting.md - new §5.6 hook lifecycle contract - four new public headers added to §5.5 header tree specs/tasks/_index.md - M5 milestone row updated - 8 task-status rows (045..052) - dependency-graph branch - PRD-HOOK coverage rows - DR-012 coverage row Per-route hooks (TASK-051) are restricted to phases that fire after route resolution. v1 alias retention is covered in TASK-048 (404/405/auth), TASK-049 (internal_error_handler), TASK-050 (log_access), and re-documented in TASK-052. TASK-052 explicitly touches back into the already-Done TASK-040 (examples), TASK-041 (README), TASK-042 (RELEASE_NOTES), TASK-043 (Doxygen) — the planned M6 touch-back called out when this scope was approved for inclusion in PR #374. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eview Major fixes (all 5 addressed): - Replaced bare catch(...) on unregister_path with catch(invalid_argument) + catch(exception) with stderr logging — surface unexpected exceptions. - Added 5 ms inter-request sleep to curl client loop — rate-limits to ~200 req/s under TSan, keeping wall-clock within CI budget. - Added null check after curl_easy_init() — skip thread gracefully on resource exhaustion rather than crash. - Added early return after fork() failure (LT_CHECK(child >= 0)) — prevents waitpid(-1,...) from reaping unrelated processes. - TASK-032.md status already Done; assertions already individual (TASK-052). Minor fixes (cosmetic + docs): - Added kDynSlots, kIpRange, kExitCodeStopReturned, kExitCodeCurlCompleted named constants; replaced all hex magic literals (0x7, 0xff, 42, 43). - Reduced CURLOPT_TIMEOUT_MS in stop-from-handler child to 3000L (< parent's 5 s window) so timeout ordering is consistent. - Renamed deadline → child_deadline in stop_from_handler test. - Replaced std::string(run) with std::string_view(run) for env-var check. - Added upper bound (v <= 3600) to stress_seconds() (CWE-1284). - Added comment documenting why register_prefix is intentionally excluded. - Added @see stop() cross-reference to ~webserver() Doxygen. - Updated specs/architecture/09-testing.md §9 item 6 to use v2 API names. - Corrected DR-008.md Verification: exit codes are regression sentinels, not positive observations; WIFSIGNALED paths are the positive observations. - Added ~webserver() note to DR-008.md Verification section. - Added PRD-CON-REQ-001/002 EARS concurrency requirements to product_specs.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5 majors: 4 fixed in this batch, 1 already addressed. 33 minors: 24 fixed/already addressed, 4 deferred (low-value churn). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix or document all 43 pending minor findings from the 2026-05-18 review pass. No behavioral changes — cosmetic, documentation, and low-risk structural improvements only. Key changes: - Fix typo in webserver_callbacks.cpp (finding 22: 'Asssume/enought') - Add `*~` to .gitignore to suppress editor/autoconf backup files (34) - Add class-level comment to modded_request (18) - Trim verbose DR-010 comment on response field to one line (28) - Update lambda_resource: note render_connect/render_trace fallback (27), add slot-type doc comment (17), add invoke_() doc comment (26) - Trim duplicate inline comment from http_resource::render() (29) - Add MHD refcounting comment near MHD_destroy_response for deferred responses explaining why the immediate destroy is safe (15) - Rename materialize_current -> try_materialize (23) - Extract emplace_and_materialize helper in get_raw_response_with_fallback to fold repeated emplace+materialize pattern (30) - Wrap ws_upgrade_data in unique_ptr RAII guard in websocket upgrade path; release to MHD after queue_response (42 / CWE-401) - Document CWE-532 contract on log_dispatch_error (44) - Document auth oracle design choice (found==true gate, 43 / CWE-200) - Reduce thread_safety test sleep from 10s to 2s and add liveness comment on tautological assertion (49) - Rename deferred_response_suite test -> deferred_response_with_prefix_content (50) - Add on_post_lambda_returns_value integration test to guard non-GET lambda dispatch path (51) - Condense lengthy AC-3 comment to observable contract (52) - Add double-stop idempotency note in AC-3 test (25) - Replace default_render_returns_empty unit test with default_render_returns_sentinel which tests the actual -1 sentinel contract introduced by TASK-036 (53) - Add route_entry::lambda_handler dispatch note for PRD-RSP-REQ-007 (47) - Update http-resource.md arch doc: render_* return by value, DR-004 (36) - Add per-slot copy behaviour comment in commit_handlers_to_shim (39) Deferred (unchanged): 13, 37, 38, 40, 41, 45, 46 (already documented or design-level decisions); 20, 21, 31, 32, 33 (already addressed by code refactoring in prior commits); 16, 35 (no action needed). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Major (item 1): replace redundant `catches_as_feature_unavailable_directly`
test (which only proved pointer upcast already guaranteed by static_assert)
with `catches_as_std_exception`, a distinct test verifying the full
std::exception -> std::runtime_error -> feature_unavailable chain
and that what() is not sliced.
Other fixes:
- src/httpserver.hpp: bump C++ guard from 201703L (C++17) to 202002L
(C++20) per DR-001 (item 2)
- feature_unavailable.hpp: replace magic literal 32 with
`k_fixed_overhead` constexpr computed from the actual string literal
at compile time, keeping reserve() in sync with future wording
changes (items 3, 15); inline compose_message helper into an
immediately-invoked lambda in the constructor initializer, removing
private surface (item 9); add @param lifetime doc comments for the
string_view arguments (item 16)
- feature_unavailable_test.cpp: add `LT_CHECK(!msg.empty())` guard
before find-checks in all tests (item 6); add
`empty_args_produce_non_empty_message` test for ("", "") edge case
(items 7, 19); add comments to empty set_up/tear_down bodies (items
4, 5, 11, 12)
Deferred: item 14 (_index.md status, handled by merge step),
item 17 (pre-existing HAVE_* guards, future task per PRD-FLG-REQ-001),
item 10 (reviewer says no change needed).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Items 1-3 from 2026-05-02_230828_task-005.md: - set_all_then_contains_every_method: replace loop with explicit per-method LT_CHECK calls for all 9 methods (GET through PATCH) - clear_all_makes_empty: same explicit enumeration, drop loop - complement_of_singleton_contains_every_other_method: split into two tests: complement_of_singleton_excludes_that_method (asserts absent) complement_of_singleton_contains_every_other_method (asserts each of 8) All 13 runtime tests pass; static_asserts compile clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Makefile.am: add comment explaining HEADER_HYGIENE_FORBIDDEN anchor semantics (item 8); add wildcard parse-time warning comment and broaden HYGIENE_STAMP to include top-level src/httpserver.hpp (items 10, 29); normalize variable alignment; add inline override comment to HEADER_HYGIENE_STRICT (items 22, 19/23); expand check-local comment to list all sub-checks and document CHECK_*_SHARED knob (items 14, 21); replace awk with sed for robust path extraction (item 24); add check-local to .PHONY (item 25); quote abs_top_builddir in shell word positions (item 37); add staged-install content guard after test -d check (item 46) - verify-build.yml: exclude header-hygiene from 'Print tests results' step (item 42); update stale comments about HEADER_HYGIENE_STRICT default - TASK-007.md, TASK-020.md: fix gnutls acceptance-criteria pattern from gnutls\.h to gnutls/gnutls\.h (items 40, 26) - header_hygiene_test.cpp: add _WINPTHREADS_H guard for MSYS2/MINGW64 (item 48); document deliberate multi-assert design (item 18); document <cstdio> inclusion rationale vs consumer_umbrella_no_backend.cpp (item 5); update guard-macro table with MSYS2 variant Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d consumer TU After editing HEADER_HYGIENE_FORBIDDEN or consumer_umbrella_no_backend.cpp, the stamp should be invalidated so check-hygiene re-stages the install. Addresses item 43 from the TASK-007 review. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Major (both already addressed by earlier tasks, now marked):
- Item 1: redundant constants.hpp include in http_utils.cpp — already gone
- Item 2: explicit std::string{} wrapping in webserver.cpp — already gone
Applied in this batch:
- src/webserver.cpp: remove redundant #include "httpserver/constants.hpp"
(transitively available via create_webserver.hpp; item 25)
- src/httpserver/create_webserver.hpp: uint16_t → std::uint16_t for _port
field, constructor, and port() setters (item 5)
- src/httpserver/constants.hpp: trim verbose per-constant comments to single
focused lines; condense namespace comment; remove internal ticket refs
(items 8-10, 19-21); fix inaccurate "non-negative by construction" claim
(item 22); document METHOD_ERROR name-preservation rationale inline (11, 23)
- specs/architecture/05-cross-cutting.md: add constants.hpp to §5.5 header
layout table (item 4)
- specs/tasks/M1-foundation/TASK-006.md: clarify acceptance criterion grep
scope — exclude include guards, COMPARATOR, platform shims (items 27, 31)
- test/unit/constants_test.cpp: strengthen comment explaining intentional
static_assert / runtime duplication (items 15, 35)
Deferred: items 14/16/26/34 (framework-required empty set_up/tear_down),
28 (string_response string_view overload — separate task), 30 (sub-namespace
API decision), 32 (COMPARATOR/platform-shim cleanup — future task),
33 (negative-compile test — build system change).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All 26 items in specs/unworked_review_issues/2026-04-30_233954_task-001.md now have [x] with Status annotations: Fixed in this batch (329aad9): items 1, 2, 4, 9, 12, 15 - Item 1: IWYU clone switched to --depth 1 --single-branch, SHA logged, TODO added - Item 2: curl SHA logging + TODO added; libmicrohttpd already pinned (TASK-037) - Item 4: ChangeLog updated gcc >= 10 -> gcc >= 11 - Item 9: configure.ac comment added about AX_CXX_COMPILE_STDCXX injection - Item 12: CI matrix drop comments rewritten with "(were here)" placement hint - Item 15: README.CentOS-7 updated to gcc >= 11 Already addressed before 329aad9: items 3, 6, 7, 8, 13, 18, 19, 23, 24 - Items 3/6/7/18: performance/lint matrix already at gcc-14 - Item 8: noext/mandatory recommendation said "no change needed" - Item 13: CXXLAGS typo fixed in TASK-038 - Item 19: permissions block added in TASK-037 - Items 23/24: recommendations said "no action required" Deferred (no action possible/needed in this batch): items 5, 10, 11, 14, 16, 17, 20, 21, 22, 25, 26 - Items 5/17: pre-existing AM_CFLAGS pattern, explicitly out of scope - Item 10/16: README formatting/structure change (minor cosmetic, low priority) - Item 11: upstream vendored m4 macro; modifying testbody causes drift - Item 14/26: IWYU hardcoded -std=c++20 literal; non-trivial to derive at CI time - Item 20: GitHub Actions SHA pinning requires Renovate/Dependabot follow-up - Item 21: pre-existing CXXFLAGS quoting pattern; low risk on GitHub Actions - Item 22: CHECKSUMS sidecar for m4 vendor file; follow-up supply-chain task - Items 25: MinGW version check nice-to-have; AX_CXX_COMPILE_STDCXX gates it
…ew tests) Header changes (http_method.hpp): - Rephrase "consteval-friendly" comment → "constant-expression" (item 7) - Add inline comment to operator&(method, method) documenting singleton AND semantics — non-empty only when a == b (item 8) - Tighten static_assert from <= 32 to < 32; add UB explanation comment (items 9, 10, 27 — prevents potential shift-by-32 UB as enum grows) - Update method_bit() comment to accurately describe the invariant (item 10) Test file changes (http_method_test.cpp): - Tighten AC #2 static_assert bound to < 32 to match header (items 15, 22) - Add CRTP explanation comment to set_up/tear_down stubs (items 12, 20, 21) - Add test 14: compound_assign_method_set_rhs — exercises |=, &=, ^= with method_set RHS in isolation (item 14 / finding 33) - Add test 15: mixed_set_method_operators_are_commutative — covers (http_method op method_set) and (method_set op http_method) commutativity for |, &, ^ (finding 34) - Add test 16: algebraic_identity_and_annihilator_laws — s|empty==s, s&full==s, s^s==empty, s|full==full (finding 35) All 16 runtime tests pass (80 checks, 0 failures). Static asserts compile clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 major + 20 minor items marked [x] fixed; 12 minor items marked deferred. Fixes applied in fix/task-005-review-cleanup branch (2 commits). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The install_not_found_alias_ hook body was previously labelled "structurally deferred (TASK-048)". TASK-048 shipped; the deferral comment was a misread of the design pinned in DR-012 §4.10 — route_resolved is observation-only, so the on-wire 404 body intentionally flows through webserver_impl::not_found_page (the v1 call site), and the alias seat at this phase exists as the architectural anchor required by PRD-HOOK-REQ-009 (v1 setters register as hook-bus subscriptions). The body is upgraded from a stub-with-debt-comment to a documented no-op observation marker that explicitly does NOT re-invoke the user handler (avoiding double-counting of v1 observed call rates). Pin the wire contract end-to-end with hooks_not_found_alias_test: - default branch (no explicit handler) returns 404 "Not Found", - custom branch returns the user handler's body once per miss, - a user route_resolved observer co-fires alongside the alias seat (DR-012 §4.10 ordering — observation phase is not short-circuit- capable). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…da_handler arm route_entry::handler was originally `std::variant<lambda_handler, shared_ptr<http_resource>>`. The lambda_handler arm has been dead code since TASK-053: every writer of route_entry (on_*/route entry points, register_path, register_prefix) funnels lambdas through a lambda_resource shim and stores the shim as shared_ptr<http_resource>. No call site stored a lambda directly, and no v2 task introduced a writer that would. Both readers (resolve_resource_for_request, prepare_or_create_lambda_shim) already treated the lambda arm as unreachable. Collapse handler to a bare shared_ptr<http_resource>: - route_entry.hpp drops std::variant + the lambda_handler typedef; - the lambda_handler typedef relocates to detail/lambda_resource.hpp where its remaining uses live (the per-method slot signature); - the dispatch site (webserver_dispatch.cpp::resolve_resource_for_request) reads result.entry.handler directly with a defensive null check; - prepare_or_create_lambda_shim and update_existing_v2_entry shed the std::get_if indirection; - the routing_regression unit pin updates likewise. Pin the new shape with a static_assert in webserver_on_methods_test: TASK-071 history is captured in the assertion message so a future reintroduction of variant storage trips an early-warning canary at compile time. The webserver::route(...) and on_*(...) public signatures take std::function (not std::variant) and were unaffected — no Doxygen update needed on the public surface. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…nd_alias_test Ports 8211-8213 collided with webserver_on_methods_test (PORT+21..23); shift to 8214-8216. Replace sleep_for(50ms) with a CURLOPT_CONNECT_ONLY retry loop (2 s deadline) so tests survive loaded CI hosts without sending spurious HTTP traffic that would skew not-found-handler counts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Mark TASK-071 Status Done and tick all four action-item checkboxes. Update _index.md TASK-071 row from Backlog to Done. Update route-table.md to reflect that route_entry::handler is now a bare std::shared_ptr<http_resource> (lambda_handler variant arm removed); append TASK-071 note to the Implementation status paragraph. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…handler variant arm Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Route the GET-argument unescape output through the per-connection arena so the warm-path zero-global-allocation guarantee holds unconditionally -- previously the v1 code path materialised the unescape result in a std::string temporary, paying one global-heap allocation per argument when the value exceeded std::string's SSO threshold (TASK-018 acknowledged deferral). build_request_args now allocates the destination pmr::string directly in the arena domain and runs the standard %HH / '+' decode in place via a TU-local http_unescape_raw helper. When a user unescaper is registered the ABI-locked `void(std::string&)` callback is fed a thread_local std::string scratch buffer whose capacity amortises across requests on the same worker thread, so the steady-state per-request global-heap cost is zero on the user path too. Tests pinned in test/unit/http_request_unescape_arena_test.cpp: the headline pair use a global operator-new counter (RAII window guard) to assert that build_request_args makes zero global-heap allocations on the warm cycle for both the default and the user-registered unescaper paths. Correctness + lifetime tests cover %2F decode, invalid-hex passthrough, empty input, view-outlives-subsequent-inserts, and the user-callback grow path. bench_warm_path gains two variants: (5) %2F-containing value through the arena-routed unescape and (6) no-escape baseline. The two medians land within noise (99 ns vs 102 ns on the debug build), satisfying the "matches the no-unescape baseline within noise" acceptance criterion. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The old bench accumulated all 1M values under a single key without resetting the arena between outer rounds. The 65536-byte arena was exhausted after ~819 iterations (65536 / ~80 bytes per pmr::string), so every subsequent emplace_back spilled to the upstream heap — the bench was measuring heap-allocation cost in steady state, the exact overhead TASK-072 was supposed to eliminate. Restructure bench sections (5) and (6) to mirror the production per-request lifecycle: each measured call creates a fresh arena-backed impl, inserts one arg, and destroys the impl. With a 65536-byte arena and a single insert per call, all pmr::string allocations stay inside the arena and the bench now proves the zero-heap-alloc guarantee. (5) and (6) now report indistinguishable medians (~1815 ns vs ~1817 ns) confirming that the %2F-decode path adds no heap overhead over the no-escape baseline. Addresses: performance-reviewer-iter1-1 (critical) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Both src/http_utils.cpp and src/detail/http_request_impl.cpp kept
private copies of hex_digit_value and the core %HH/'+' decode loop,
each with an independent comment acknowledging the duplication.
Promote both helpers to src/httpserver/detail/unescape_helpers.hpp:
- hex_digit_value: constexpr int, inlined
- unescape_buf_raw: inline raw-buffer unescape (char*, size_t) -> size_t
http_unescape in http_utils.cpp is now a thin wrapper that calls
unescape_buf_raw and adds the trailing '\0' expected by callers of
the std::string* variant.
http_request_impl.cpp's anonymous http_unescape_raw is removed; its
sole caller (unescape_in_arena) now calls unescape_buf_raw directly.
Also addresses:
- Rename unescape_in_arena parameter 'sink' -> 'value' and
thread_local 'user_scratch' -> 'thread_value' for naming
consistency. (code-simplifier-iter1-5)
- Document the per-thread amortisation contract and arena-waste
behaviour of the user-callback path. (performance-reviewer-iter1-2,
performance-reviewer-iter1-3)
- Collapse the long call-site comment in build_request_args to a
single cross-reference line. (code-simplifier-iter1-7)
Addresses: code-simplifier-iter1-1 (critical), code-simplifier-iter1-2
(critical), code-simplifier-iter1-5 (major), performance-reviewer-iter1-2
(major), performance-reviewer-iter1-3 (major)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three improvements to http_request_unescape_arena_test.cpp: 1. decode_via_arena(const char*) helper: extracts the shared setup boilerplate (arena+impl+aa+build_request_args+teardown) from correctness tests (3)-(5). Each test body is now a single LT_CHECK_EQ assertion. (code-simplifier-iter1-3) 2. run_alloc_pin(unescaper_ptr, int warmup_rounds) helper: extracts the shared setup from the two headline zero-alloc-pin tests (1)/(2). The warmup_rounds parameter makes the reason for the different cold- call counts between the default-unescaper (1 warmup) and user- unescaper (2 warmups) paths explicit. (code-simplifier-iter1-6) 3. Suite comment: annotate the empty set_up/tear_down stubs as intentional (the per-test lifecycle lives in the helpers), removing the structural ambiguity. (code-simplifier-iter1-4) All 8 tests pass; 12 assertions (down from 16 because tests (3)-(5) are now single-assertion calls to decode_via_arena). Addresses: code-simplifier-iter1-3 (major), code-simplifier-iter1-4 (major), code-simplifier-iter1-6 (major) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The header was missing from the Autotools dist list, meaning make dist would omit it from the released tarball and break out-of-tree builds. All in-tree builds were unaffected (masking the gap). Verified via make dist + tar tzf that the file now appears in the tarball. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Snapshot of the 35 findings (0 critical, 3 major, 32 minor) surfaced by the validation-loop reviewers that were deemed out of scope for this task. Mirrors the TASK-071 persistence pattern so they remain visible for future follow-up without blocking the merge. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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>
…ral rationale
The MHD_OPTION_UNESCAPE_CALLBACK registration in webserver_setup.cpp
installs webserver_impl::unescaper_func as a no-op pass-through. The
existing comment framed this purely as a libmicrohttpd v0.99 workaround
for an unescape bug where the internal MHD percent-decoder could produce
strings containing embedded NULs (e.g. from %00), which then broke
MHD_get_connection_values / MHD_lookup_connection_value lookups
downstream.
Verified against the upstream libmicrohttpd ChangeLog
(/opt/homebrew/Cellar/libmicrohttpd/1.0.5/ChangeLog) that upstream
resolved the embedded-NUL-in-key/value-storage class of bug by adding
explicit binary-zero-aware key/value storage and the size-carrying
MHD_KeyValueIteratorN callback. The relevant ChangeLog entries are:
Wed 20 Mar 2019 Adding additional "value_length" argument to
MHD_KeyValueIterator callback to support binary zeros
in values. -CG
Wed May 1 2019 Implemented MHD_KeyValueIteratorN with sizes for
connection's key and value to get keys and values
with binary zeros. -EG
Fri May 3 2019 Added functions for working with keys and values
with binary zeros. -EG
Sun Jun 09 2019 Releasing libmicrohttpd 0.9.64. -EG
configure.ac requires libmicrohttpd >= 1.0.0 (released 2024-02), which
is five years and dozens of releases past 0.9.64, so the v0.99 bug is
no longer reachable on any supported MHD version.
However, the no-op callback registration must stay. Per microhttpd.h
1849-1872 (MHD_OPTION_UNESCAPE_CALLBACK doc), registering a custom
unescape callback suppresses MHD's internal "%HH" decode. libhttpserver
relies on that suppression for two independent reasons:
1. Honouring a user-registered unescaper hook
(create_webserver::unescaper(...)). If MHD pre-decoded behind our
back, the user callback would run on already-decoded input — the
custom_unescaper integ test (test/integ/basic.cpp:2179) pins this.
2. Routing GET-arg decoding through the per-connection PMR arena
(unescape_in_arena() in src/detail/http_request_impl.cpp:265, wired
by TASK-072). Internal MHD decode would heap-allocate and double-
decode.
So this task does not remove the registration; it rewrites the comment
to make the architectural justification durable and demote the v0.99
bug-fix framing to a historical footnote with a verified version
cutoff. No #if MHD_VERSION guard is added: configure.ac already
enforces the floor at build time, so a runtime guard would be dead
code.
No behavioural change. The three pinning suites called out in the
plan stay green:
- test/unit/http_request_unescape_arena_test.cpp (8 tests / 12 checks)
- test/unit/http_request_arena_test.cpp
- test/integ/basic.cpp (custom_unescaper at line 2179, plus 96 others)
Full test/ suite: 100/100 PASS.
The task file already shows Status: Complete; bring the _index.md task table into sync by flipping the TASK-073 row from Backlog to Done. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All findings are minor (0 critical, 0 major); they describe documentation nits in the new comment block and pre-existing out-of-diff inconsistencies in webserver_request.cpp comments. No behavioural concerns. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Documents the architectural rationale for keeping the no-op MHD_OPTION_UNESCAPE_CALLBACK registration — needed to suppress MHD's internal %HH decode so the per-connection PMR arena (TASK-072) and any user-registered unescaper hook own decoding end-to-end. The v0.99 bug framing is demoted to a historical footnote with a verified version cutoff (libmicrohttpd 0.9.64, 2019-06-09). No behavioural change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replaces the #ifdef DEBUG guard around the raw request-body stdout
dump in src/detail/webserver_body_pipeline.cpp with a runtime opt-in
keyed on LIBHTTPSERVER_DEBUG_DUMP_REQUEST_BODY. Default behaviour is
now silent on RELEASE *and* DEBUG builds alike -- the env var is the
only gate, so a debug build accidentally shipped to production cannot
leak credentials/PII unless the operator explicitly opted in.
When the env var is observed at webserver::start(), the library emits
a single SECURITY WARNING line to stderr (and to the user's log_error
callback when wired) naming the env var, the credential/PII risk, and
the docs pointer. Process-wide std::atomic<bool> flag means multiple
webservers in the same process produce exactly one stderr emission.
The runtime gate is cached in a function-local static (Magic Static)
so getenv() is read at most once per process; subsequent setenv()
calls are intentionally ignored (debug-knob semantics).
New documentation:
- docs/debug-env-vars.md - canonical home for LIBHTTPSERVER_*
debug knobs; describes effect, risk, startup signal, disable
procedure, and the spot-check that release-with-DDEBUG still
behaves correctly. Includes a checklist for future debug knobs.
- specs/architecture/10-observability.md cross-references the new
doc.
New tests (test/integ/):
- debug_dump_request_body_unset_test.cpp - asserts silence on both
stdout and stderr when the env var is unset; passes identically
on DEBUG and RELEASE builds.
- debug_dump_request_body_set_test.cpp - asserts the body dump
and one-shot SECURITY WARNING fire when the env var is set, and
pins the process-wide idempotence with a second webserver.
The two binaries are split because the function-local-static cache
means the first observation in the process locks in for the rest;
splitting gives each scenario a deterministic fresh process.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… issues Mark TASK-074 status as Done in the task file and milestone index, and record the 28 review findings (6 major, 22 minor) that were not actioned during this task so they can be picked up by a follow-up cleanup pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Extract the readiness helper introduced in TASK-049 from
hooks_handler_exception_chain.cpp into a new shared header
test/integ/server_ready.hpp (namespace httpserver_test) and migrate the
22 sibling hooks_*.cpp files (plus the source file) from the racy
std::this_thread::sleep_for(50ms) pattern to the deterministic
httpserver_test::wait_for_server_ready(PORT) call.
Probe strategy: the shared helper uses CURLOPT_CONNECT_ONLY (TCP-only).
This matters because a HEAD-probe variant — naive port of the
exception_chain inline helper — would have fired the HTTP-level hook
phases (request_received, route_resolved, before_handler,
after_handler, response_sent, request_completed) on the server under
test, breaking exact-count assertions in
hooks_response_sent_carries_status_bytes_timing,
hooks_per_route_order, hooks_route_resolved_miss_and_hit and
hooks_request_completed_fires_on_early_failure. The TCP-only probe
also works on banned-IP tests (hooks_accept_decision_banned,
hooks_accept_decision_throwing banned sub-case) because the kernel
completes the three-way handshake before MHD's policy_callback runs.
Also consolidates hooks_not_found_alias_test.cpp's near-duplicate
wait_for_server helper to the shared header (it was the source of the
TCP-only design adopted here).
Add scripts/check-server-ready-helper.sh, a bash + grep/awk regression
gate that scans test/integ/hooks_*.cpp for any bare server-ready
sleep_for call and exits 1 if found. The gate honours a per-line
'// NON-READINESS-SLEEP: <reason>' escape hatch for legitimate
non-readiness sleeps; the self-test (scripts/test_check_server_ready_helper.sh,
5 cases, modelled on test_check_warning_suppressions.sh) verifies the
detection logic, including the empty-watched-files edge case that the
${arr[@]+...} idiom guards against. The gate is wired into
check-local in Makefile.am (alongside lint-warning-suppressions), so
it runs on every CI lane via `make check`.
Removes the now-unused <thread> and <chrono> includes from every
migrated TU except hooks_per_route_concurrent_registration.cpp (which
still uses std::thread for its worker pool).
Verified locally: all 102 integ tests pass; the gate self-test
reports 5/5; the gate against the migrated tree returns 0
violations (against the pre-migration tree it returned 24, matching
the audit). Doxygen check-doxygen is failing on this branch
pre-existing (unrelated to TASK-075).
Closes TASK-075 (M7 - v2 Cleanup).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 major (test-quality-reviewer flags scripts/test_check_server_ready_helper.sh as unreachable from `make check` because it is neither in EXTRA_DIST nor wired into a check-local target) and 18 minor findings deferred for future cleanup. See specs/unworked_review_issues/2026-06-09_103302_task-075.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`test/integ/ws_start_stop.cpp` carried 33 `LT_CHECK_EQ(1, 1)` statements
that simulated a "skip" inside `try { ws.start } catch { ... return; }`
or `if (!has_gnutls_cli()) { ... return; }` blocks. On hosts without
GnuTLS or `gnutls-cli`, the entire TLS suite reduced to passing
tautological assertions — a build that silently lost TLS support would
still report green CI.
Added `LT_SKIP` / `LT_SKIP_IF` macros and `skip_unattended`
exception class to `test/littletest.hpp`. `test_runner` now tracks a
`skip_counter`, prints `-> N skipped`, and returns 77 (Automake's SKIP
code) when a binary's only outcomes are skips. Two new gates pin the
plumbing: `lint-no-tautological-asserts` (forbids regressing the
pattern in ws_start_stop.cpp) and `lint-littletest-skip-exit-code`
(asserts the exit-77 / mixed-pass+skip-0 semantics). Both wired into
`check-local`.
Replaced all 33 occurrences with one of:
(a) `LT_SKIP_IF(true, "TLS start failed: ...")` in the 11 TLS-start
catches; reports SKIP rather than tautological PASS.
(b) Typed `catch (const std::exception&)` + `is_psk_unsupported_error`
substring match in the 8 PSK catches: SKIP on
libmicrohttpd-built-without-PSK; `throw;` (FAIL) otherwise so
implementation-broken cases surface.
(c) Strengthened 4 tail-position liveness sentinels: 3 ×
`LT_CHECK_EQ(ws.is_running(), false)`; 1 ×
`LT_CHECK_NEQ(cmd_exit, 127)` (capturing `system()`'s result
instead of discarding it).
(d) Deleted 3 redundant tail-position tautological assertions.
The five `if (!has_gnutls_cli())` env-gates now use
`LT_SKIP_IF(!has_gnutls_cli(), "gnutls-cli binary not in PATH")`.
Wired CI: new `tls-no-cli` lane installs `libgnutls28-dev` WITHOUT
`gnutls-bin`; post-test verification step greps
`build/test/ws_start_stop.log` for >=5 `[SKIP]` markers and zero
`[CHECK/ASSERT FAILURE]` from the five `psk_connection_*` tests. A
sibling canary on the baseline `classic` lane asserts those tests do
NOT report SKIP when `gnutls-bin` IS present, catching detection
regressions.
Acceptance criteria:
* `grep -nE 'LT_CHECK_EQ\(1, *1\)' test/integ/ws_start_stop.cpp`
returns no matches.
* Local run with PATH masked to drop gnutls-cli: ws_start_stop emits
5 SKIPs (the psk_connection_* tests) and exits 0 with 112
successes + 0 failures preserved. With gnutls-cli on PATH the
binary runs all 50 tests (117 successes, 0 failures, 0 skips).
* `make check` in test/ stays green at 103/103 PASS.
Files:
* test/littletest.hpp — SKIP severity, exception class, runner
counter + exit-77 plumbing.
* test/integ/ws_start_stop.cpp — 33 replacements +
`is_psk_unsupported_error` helper.
* test/unit/littletest_skip_semantics_test.cpp — TDD entry-point
sentinel pinning the runtime semantics.
* scripts/check-no-tautological-asserts.sh,
scripts/test_check_littletest_skip_exit_code.sh — new gates,
wired into check-local.
* .github/workflows/verify-build.yml — `tls-no-cli` matrix lane +
verification steps.
* RELEASE_NOTES.md — "Test infrastructure" section on the new
skip semantics.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Apply iter-1 validation-fixer changes for the three majors raised by test-quality-reviewer: - Remove unreachable dead assertions in littletest_skip_semantics_test. - Tighten is_psk_unsupported_error coverage via direct unit assertions. - Replace silent always-green branches in IPv6/dual-stack success paths with LT_ASSERT_EQ on curl results. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Capture the residual minors and unfixed code-simplifier majors from the validation loop. No critical findings; 2 majors and 26 minors logged for future cleanup sweeps. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replaces ~25 LT_CHECK_EQ(1,1) tautologies in ws_start_stop.cpp with LT_SKIP / LT_SKIP_IF (new littletest macros, Automake exit-77 semantics) or typed std::exception catches that distinguish "config unsupported" from "config broken". Adds tls-no-cli CI matrix lane and a regression gate (lint-no-tautological-asserts) wired into make check-local. Includes iter-1 review-cleanup fixes (test-quality-reviewer majors) and 28 persisted unworked findings. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three whole-suite or near-whole-suite `#ifndef _WINDOWS` / `#ifndef DARWIN` blocks in test/integ/ were silently removing the library's Windows / Darwin portability claim from CI without any commentary on why. This change: - Adds `scripts/check-skip-rationales.sh` (lint) + `test_check_skip_rationales.sh` (10-case fixture) that enforce a `// reason:` comment within 5 lines of every `#ifndef _WINDOWS`, `#ifndef DARWIN`, or `#if !defined(_WINDOWS)` block under `test/integ/`. `#ifdef _WINDOWS` (additive coverage) is deliberately not gated. - Annotates every existing skip site (`threaded.cpp` x4, `ws_start_stop.cpp` wide skip and `custom_socket`, `authentication.cpp` digest-auth block, and `connection_state_body_residue_test.cpp`) with the required comment. - Adds `test/PORTABILITY.md` recording symptom / root cause / restoration plan per skip site; the `// reason:` lines point at the relevant section. - Adds a `ws_start_stop_suite::windows_smoke` test under `#ifdef _WINDOWS` to restore a single non-TLS HTTP GET round-trip on the MinGW64 / MSYS lanes — recovering the most valuable bit of Windows coverage without inheriting the TLS / IPv6 / SNI / PSK flake tracked in PORTABILITY.md. - Wires the lint into `verify-build.yml`'s existing `lint` lane and adds a Windows-lane assertion that grep's the test log for `windows_smoke` so a future build-flag tweak that suppresses the `#ifdef _WINDOWS` branch fails CI explicitly rather than silently erasing coverage. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…iew findings Flip TASK-077 status to Done in TASK-077.md and _index.md, check off the five action items, and log the 27 minor findings (0 critical, 0 major) surfaced by the validation loop for future cleanup sweeps. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…m skips Adds `scripts/check-skip-rationales.sh` lint (with a 10-case fixture) that fails CI if a `#ifndef _WINDOWS` / `#ifndef DARWIN` / `#if !defined(_WINDOWS)` block under `test/integ/` lacks a `// reason:` comment within 5 lines. Annotates every existing skip site, adds `test/PORTABILITY.md` (symptom / root cause / restoration plan per skip), and restores a single Windows non-TLS HTTP smoke test (`ws_start_stop_suite::windows_smoke`) to recover the most valuable bit of Windows coverage. Wires the lint into the verify-build.yml lint lane and adds a Windows-lane assertion that grep's the test log for `windows_smoke` so a future build-flag tweak that silently suppresses the `#ifdef _WINDOWS` branch fails CI. Includes the status flip to Done and 27 persisted minor review findings (0 critical, 0 major) for future cleanup sweeps. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Integration branch for the v2.0 modernization effort. Tasks land here individually (one merge commit per task) so the full v2.0 ships as a single reviewable PR.
This PR will remain draft until all milestones are complete.
Milestones
Specs live under
specs/(product_specs, architecture, tasks).Merged tasks
Test plan
Per-task validation runs through the groundwork validation loop on each task branch before merging here. Pre-merge of v2.0 to
master:./configure && makeclean on macOS (Apple Clang) and Linux (recent GCC)make checkgreen-std=c++(11|14|17)regressions in tree🤖 Generated with Claude Code