v2.0: Modernization (M1-M6, 44 tasks)#374
Draft
etr wants to merge 476 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.95% +0.92%
==========================================
Files 34 64 +30
Lines 1730 4033 +2303
Branches 697 1478 +781
==========================================
+ Hits 1177 2781 +1604
- Misses 80 353 +273
- Partials 473 899 +426
... and 56 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>
Critical: build_request_args now writes directly into arena-backed pmr::string (or scratch buffer that copies into arena), eliminating the global-heap temporary on the argument unescape path. Plus 23 additional findings (majors + selected minors) addressed across http_request_impl, connection_state, and the arena tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Final pass through the 26 minor items that survived the first cleanup batch (6e55621). Net result: 47/50 review items resolved across both batches, 3 explicitly deferred with rationale. Deferred (left [ ] with *Status:* — distinct from "not addressed"): - #44 acceptance-criteria: nice-to-have integration test for request_completed trampoline (unit-level proxy in test 8 is acceptable). - #46 redundant-test: tests have distinct traceability value. - #47 excessive-setup: cosmetic test refactor deferred. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add = nullptr default initializer to modded_request::callback field, eliminating UB per the C++ standard (findings 1 & 5: code-quality- reviewer + code-simplifier). The is_allowed guard already prevents the pointer from being invoked for unrecognized methods; this makes the contract explicit at the declaration site. - Update the comment in resolve_method_callback to remove the stale "pre-existing latent bug" note now that callback has a safe default. - Add per-test counter reset at the start of the three deferred tests that share the static counter (finding 12: test-quality-reviewer). tear_down() also resets but cannot guarantee ordering when tests run in non-default order or under skip conditions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace busy-poll (100×10 ms) in deferred_producer_destroyed_in_ request_completed with a condition_variable-based wait (finding 11: test-quality-reviewer). The destruction_sentinel now holds a mutex/cv pair and signals on destruction; the test does a timed_wait with a 5-second upper bound — fast when the invariant holds, bounded when it does not. - Trim the 6-line block comment on modded_request::response_ to a one-line cross-reference (finding 28: code-simplifier). The full DR-010 lifetime contract is documented canonically in webserver_impl.hpp and the architecture doc; repeating it in the struct adds noise. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code changes (12 files):
- src/httpserver/detail/radix_tree.hpp: trim radix_match/radix_node comments
to WHY-only; add match_root_terminus() helper eliminating empty-path
duplication; add NOTE on remove() pattern-vs-value traversal asymmetry
- src/httpserver/detail/route_cache.hpp: remove noexcept from size()/clear();
introduce kHashMix constexpr replacing magic literal; add max_entries==0
precondition check (throws std::invalid_argument, CWE-754)
- src/httpserver/detail/route_tier.hpp: wrap std::regex construction in
try/catch(std::regex_error) → rethrows as std::invalid_argument (CWE-248)
- src/httpserver/detail/webserver_impl.hpp: rename route_cache_mutex →
route_cache_mutex_; add v1/v2 section banners; add TODO(Cycle K) rename
note on route_cache_v2; add CWE-284 guard comment on lookup_v2 declaration;
regex_routes_ now stores regex_route{url_complete, compiled_re, entry}
(pre-compiled at registration time, eliminating per-miss recompile)
- src/detail/webserver_dispatch.cpp: rename route_cache_mutex_; use
std::move(result.entry/captured_params) on cache-miss path (avoids
second shared_ptr bump + vector copy)
- src/detail/webserver_register.cpp: fix lock order in unregister_impl_ and
unregister_resource — registered_resources_mutex released before table lock
and cache cleared; delegate cache clear to invalidate_route_cache() matching
register_impl_ pattern (CWE-833)
- src/detail/webserver_request.cpp: move normalize_path inside namespace
detail immediately before should_skip_auth; add NOTE documenting
pre-unescaped-path invariant and double-slash collapsing behaviour (CWE-22)
- src/detail/webserver_routes.cpp: extract upsert_v2_param_route() helper
encapsulating radix read-merge-reinsert; add __builtin_unreachable() to
unreachable radix branch in insert_fresh_v2_entry; remove trailing
comment on set_all() line; regex_routes_ push_back now moves pre-compiled
regex from classify_route_tier result
- test/unit/http_method_test.cpp: remove redundant runtime
to_string_returns_uppercase_wire_tokens (duplicate of static_asserts)
- test/unit/lookup_pipeline_test.cpp: remove redundant
cache_duplicate_insert_replaces_in_place; add lambda_route_hits_exact_tier,
lambda_parameterized_route_hits_radix_tier,
plain_path_with_regex_checking_hits_exact_tier
- test/unit/route_table_test.cpp: add cache_zero_max_entries_throws;
add radix_tree_remove_parameterized_path, radix_tree_remove_prefix_path,
radix_tree_remove_one_terminus_does_not_clear_sibling
- test/integ/route_table_concurrency.cpp: replace 500ms sleep with
kOpThreshold=10000 operation-count gate (first test); rename second test
to concurrent_wildcard_node_alloc_and_lookup_no_data_race
Review file updated (specs/unworked_review_issues/2026-05-10_230348_task-027.md):
Items #4-62 annotated with *Status:* lines.
30 fixed-in-batch, 7 already-addressed, 22 deferred.
Key fixes: pre-compiled regex tier (#4,#53), lock-order deadlock (#17,#21),
CWE-284 shadow-table documentation (#15), CWE-248 regex_error guard (#48),
CWE-754 max_entries=0 check (#46), double-slash normalization note (#16),
route_cache_mutex_ rename (#20), kHashMix constexpr (#37), noexcept removal
(#25), v1/v2 boundary comments (#26), __builtin_unreachable (#22).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add a TODO comment on auth_handler_ptr in create_webserver.hpp documenting the deferred migration to std::optional<http_response> (findings 4 & 7: code-quality-reviewer + code-simplifier). Changing the public typedef mid-milestone would cascade into centralized_authentication.cpp and authentication.cpp; scoped out. - Rename local variable auth_resp -> auth_rejection_response in the auth_handler alias hook (webserver_aliases.cpp) to clarify that null means "allow" and non-null means "reject with this response" — the minimum improvement from finding 7 recommendation (b). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use aggregate initialization {methods, handler, is_prefix=false} to
construct route_entry in insert_fresh_v2_entry, replacing two identical
3-line field-assignment blocks with a single-expression construct per
case arm. No behavior change; structure is already correct.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ings 6, 9) - Rename modded_request::response_ to response throughout all 16 call-site files (finding 6: code-simplifier). The trailing underscore incorrectly implied a private-class-member convention; modded_request is an all-public struct. Affected files: webserver_aliases.cpp, webserver_body_pipeline.cpp, webserver_callbacks.cpp, webserver_dispatch.cpp, webserver_error_pages.cpp, webserver_finalize.cpp, webserver_request.cpp, hook_handle.cpp, lambda_resource.hpp, modded_request.hpp, webserver_impl_dispatch.hpp, hook_context.hpp, http_resource.hpp, test/Makefile.am, deferred.cpp, hooks_after_handler_replaces_response.cpp. All comment references updated. - Add CWE-209 WARNING comment to internal_error_page() in webserver_error_pages.cpp (finding 9: security-reviewer). Documents that the informative default (surfacing e.what() in the 500 body) is intended for development only; operators must wire a constant-returning internal_error_handler before production deployment. - Mark findings 1-12 (all major) in the review file with fix/deferral status. 10 fixed across this and prior commits (22d8f07, 5d49d42, a11e5ee, 5d21408); finding 2 already addressed by AC-1/AC-2 in deferred.cpp; finding 10 deferred (test split has low ROI given sentinel infrastructure). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…indings Source fixes (major findings): - Extract local_map_for(MHD_ValueKind) helper on http_request_impl, eliminating verbatim switch duplication in get_connection_value() and ensure_headerlike_cache() (code-simplifier #3/#4, code-quality #15/#16) - Add mutable bool user_pass_fetched to http_request_impl (under HAVE_BAUTH); guard get_user()/get_pass() on the flag instead of username/password.empty(), matching args_populated pattern and fixing redundant MHD round-trips (code-simplifier #6) - Add mutable bool requestor_ip_cached to http_request_impl; consolidate get_requestor() to a single early-return covering cache-hit and test-request paths (code-simplifier #7 + #8/#9) - Explicit = default for http_request_impl move ctor and move-assign (code-simplifier #33) Architecture/doc fixes: - http-request.md: add historical note that TASK-015 used std::make_unique and TASK-016 wired the arena (housekeeper #34/#35/#36) - DR-003b.md: add Implementation phasing section recording the two-step landing (housekeeper #1/#2 from major section) - specs/unworked_review_issues/2026-05-04_013108_task-013.md: mark finding #4 [x] (microhttpd.h leak resolved by TASK-015) (housekeeper #38/#39) Review file closeout: - Mark all 18 resolved findings [x] with Status notes - Add deferred Status notes to all 46 remaining minor findings Findings fixed this batch: 7 (code-simplifier #3/#4/#6/#7/#8/#9/#33 + code-quality #15/#16 + housekeeper #34/#35/#36/#38/#39 = 14 unique findings across both review-file and source changes) Findings deferred (minor, all annotated): 46 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- scripts/lib/resolve-prefix.sh: new shared helper sourced by both gate scripts; POSIX-portable sed (no GNU \?), absolute-path guard, split pipeline for readability. Eliminates DRY violation (items 2, 3, 10, 16, 22). - check-soversion.sh: move resolve_symlink_chain() to top alongside fail/pass (item 4); add set -uo pipefail comment (item 9); tee install for streaming CI output (item 11); strict SONAME equality in A5 via sed bracket-extraction (item 27); narrow A7 grep to ^library_names= field (item 28). - check-parallel-install.sh: fix header comment Darwin name (item 6); add -e-omission comment (item 9); extract remove_master_worktree() helper for EXIT trap and stale-cleanup (item 14); rename V1_PATTERN to V1_SONAME_PATTERN with per-platform comments (items 13, 24); gate Homebrew flags on Darwin, use brew --prefix dynamically (items 8, 15, 26); dynamic make -j parallelism (item 19); Phase 3 strict exact-filename check from .la library_names field, SONAME collision detection, while IFS= read -r for v1_hits (items 1, 5, 21). - configure.ac + src/Makefile.am: move -version-number from global LDFLAGS to libhttpserver_la_LDFLAGS via LHT_LDF_VERSION AC_SUBST (item 25). - Makefile.am: add scripts/lib/resolve-prefix.sh to EXTRA_DIST. - specs/tasks/_index.md: bump Last updated to 2026-05-20 (items 12, 18). - Mark 27/28 items in review file; item 23 deferred (design decision). Co-Authored-By: Claude Sonnet 4.6 <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>
…on-time-only Option (b) of the task spec: rather than add a runtime alias setter (no demand signal, two-mechanisms-behind-one-façade asymmetry, DR-012 already frames aliases as construction-time sugar), remove the speculative "if a future task adds a runtime setter..." comments and pin the immutable-after-construction contract with tests and documentation. - src/hook_handle.cpp: trim three forward-debt comment blocks (in erase_slot_for_handler_phase_, fire_handler_exception tail, fire_response_sent tail) to one-line "immutable after start() (DR-012 §4.10)" rationale. - src/detail/webserver_aliases.cpp: trim the install_internal_error_alias call-site comment to the immutable-after-construction one-liner. - src/httpserver/detail/webserver_impl.hpp: trim the lifetime-contract comments on handler_exception_alias_ and log_access_alias_ to drop the speculative "future runtime setter" paragraphs; keep the single-writer-at-construction rationale. - src/httpserver/hook_handle.hpp: add an "Alias slots are construction-time-only" docstring paragraph naming the five v1 aliases and pointing users to add_hook() for runtime extension. - specs/architecture/04-components/hooks.md: add an "Alias mutability" paragraph to §4.10 explicitly stating that v1 aliases are construction-time-only and that the hook bus is the runtime extension surface (per DR-012). - test/unit/hooks_log_access_alias_slot_test.cpp: replace the misleading log_access_second_registration_replaces_first test (the smell TASK-085 finding 3 flagged) with two real contract pins: log_access_alias_is_immutable_after_construction and handler_exception_alias_is_immutable_after_construction. Both verify that user add_hook() at the relevant phase grows the user vector but never displaces the alias slot, and that hook_handle::remove() leaves the alias slot intact. Acceptance: - grep -nE 'if a future task adds a runtime setter for the alias slots' src/ returns no matches. - Broader audit grep 'future task adds a runtime|future runtime setter|runtime re-registration path' src/ also returns no matches. - hooks_log_access_alias_slot now: 7 tests, 29 checks, 0 failures. - All hook-related tests pass; cpplint clean on changed files. - specs/tasks/M7-v2-cleanup/TASK-066.md updated to Done with resolution notes and TASK-085 handoff. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…truction-time-only
Three transitional v1 surfaces are gone, leaving the v2 3-tier route
table as the only registration / dispatch oracle:
1. webserver_impl::{registered_resources, registered_resources_str,
registered_resources_regex, registered_resources_mutex} — registration-
time bookkeeping replaced by direct writes to exact_routes_, the radix
tree (param_and_prefix_routes_), and regex_routes_. Duplicate detection
migrates from the v1 ordered map's .insert() oracle to a new
reject_duplicate_v2_entry_ helper that probes the correct v2 tier for
a pre-existing entry before any mutation — preserving the
"throw-leaves-table-prior-state" atomicity contract pinned by
basic_suite::duplicate_endpoints.
2. namespace httpserver::compat (auth_handler_v1_ptr +
adapt_legacy_auth), the [[deprecated]] create_webserver::auth_handler
overload accepting it, and the paired push/pop
-Wdeprecated-declarations suppression at the forwarding call site —
all removed. The auth_handler_legacy_shim_test TU goes away; a new
no_v1_compat_shim_test TU pins the post-removal sentinel via a
std::void_t SFINAE detection idiom (negative compile-time check that
the v1 shared_ptr-returning shape is no longer accepted).
3. Readers of the v1 maps — prepare_or_create_lambda_shim's conflict
detection, the resolve_single_resource_ short-circuit in dispatch,
and the WebSocket-path's use of registered_resources_mutex to guard
registered_ws_handlers — all rewired. WebSocket registration/dispatch
now uses a dedicated ws_handlers_mutex_ instead of piggy-backing on
the deleted v1 mutex. The single_resource fast path is folded into
lookup_v2 (a single registered prefix terminus at "/" surfaces as a
trivial radix descent).
TASK-070 (the partner -Wdeprecated-declarations suppression in
http_resource.cpp:81-115 that the action-items list points to) is
explicitly handed off — its own M-sized scope (atomic shared_ptr
migration) would balloon this PR.
Drive-by fixes surfaced by the fresh integ-test rebuild this commit
forces:
- test/integ/basic.cpp:155,367 — wrap two legacy
with_cookie(string,string) call sites in -Wdeprecated-declarations
push/pop. Pre-existing TASK-064 deprecation that incremental builds
silently skipped (basic.o was stale from before TASK-064).
- test/integ/basic.cpp:498 — duplicate_endpoints's "ok" vs "OK"
case-folding assertion was pinning a v1 quirk (registered_resources
used http_endpoint::operator<, unconditionally case-insensitive via
std::toupper regardless of CASE_INSENSITIVE). The v2 route table is
consistently case-sensitive without -DCASE_INSENSITIVE, so the
assertion is now CASE_INSENSITIVE-gated to match dispatch-time
lookup semantics.
Acceptance criteria (all satisfied):
- grep -nE 'registered_resources' src/httpserver/detail/webserver_impl.hpp
returns no matches.
- grep -nE 'namespace compat' src/httpserver/create_webserver.hpp returns
no matches.
- grep -nE 'Wdeprecated-declarations' src/httpserver/create_webserver.hpp
returns no matches.
- make -j1 check passes 95/95 tests.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace the std::memset call in connection_state::reset_arena() with a
new httpserver::detail::secure_zero helper that the optimizer is
forbidden to elide as a dead store. The helper dispatches to
explicit_bzero (glibc/musl/BSD), RtlSecureZeroMemory (Windows), or a
portable volatile-pointer loop + asm("" ::: "memory") clobber fallback
(macOS and any other lane without explicit_bzero). The full 8 KiB
initial_buffer_ is scrubbed unconditionally, closing the residual-
credential window across keep-alive requests for credentials that fit
within the arena (overflow blocks remain an accepted residue, documented
in the rewritten comment block).
The DR-008 sanity gate (a DEADBEEFCRED sentinel carried as a basic-auth
username on request 1 must not be observable to request 2 on the same
MHD connection) is pinned by a new integ test that reaches the
underlying MHD_Connection through a new HTTPSERVER_COMPILATION-gated
http_request::underlying_connection_for_testing() accessor and probes
connection_state::initial_buffer_ directly. A second integ-signal hook
(connection_opened) asserts curl keep-alive engaged exactly once across
both requests.
Two additional unit tests guard the helper itself: secure_zero_dce_test
is compiled at -O2 -DNDEBUG and reads every byte through a volatile
sink to verify the writes are not dead-store eliminated; and
connection_state_sentinel_test pins that reset_arena() zeros the entire
arena, not just the bump-pointer prefix.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adjust the body-residue integ test to force HTTP/1.1 (HTTP/2 stream multiplexing would invalidate the keep-alive precondition) and gate the headline assertion behind LT_ASSERT_EQ on connection_opened_count so a fresh connection cannot produce a vacuously-passing result. Switch the secure_zero DCE test's buffer to `volatile unsigned char[]` so the optimizer cannot propagate the pre-zeroing sentinel directly into the post-zeroing read (closes a real DCE loophole the prior sink-only approach left open). Extend the nullptr-with-zero-size test with an adjacent-sentinel guard so an accidental out-of-bounds write is caught. Flip TASK-068 in M7 _index.md to Done, and persist the 35 unworked review findings (0 critical, 2 major, 33 minor) from the final review cycle for future follow-up tasks. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The two-arg http_request_impl(MHD_Connection*, unescaper_ptr) overload
was preserved during the M3 PIMPL split (TASK-015 / TASK-016) "for
source compatibility" with callers not yet ported to the allocator-
taking form. The v2 cutover is complete and detail/ headers do not
require a deprecation cycle, so the overload comes out now.
Changes:
- src/httpserver/detail/http_request_impl.hpp: delete the two-arg
delegating constructor and its comment block. The default ctor
(test-request path) and the canonical three-arg allocator-aware
ctor remain.
- src/http_request.cpp: migrate the sole live caller (the
heap-fallback branch when pick_resource returns the default
resource) to pass std::pmr::polymorphic_allocator<>(get_default_
resource()) explicitly. Semantically identical to what the
delegating ctor did internally; byte-for-byte heap behaviour is
unchanged.
- test/unit/http_request_pimpl_test.cpp: add three static_asserts
pinning the construction surface:
1. default-constructible (test-request path);
2. canonical three-arg (MHD_Connection*, unescaper_ptr, alloc)
remains constructible;
3. negative: !is_constructible from (MHD_Connection*,
unescaper_ptr) -- the TASK-069 sentinel. If the two-arg
overload ever returns, the build breaks here.
Verification:
- `make check -j1` (release): 98/98 pass.
- `make check -j1` (--enable-debug, -Werror -Wextra): 98/98 pass.
- Header-only cross-flag sweep compiles the sentinel TU under each
of -UHAVE_BAUTH / -UHAVE_DAUTH / -UHAVE_GNUTLS / -UHAVE_WEBSOCKET
cleanly (proves the canonical ctor's #ifdef-conditioned member-
init list still works under every flag-off permutation, per the
plan's Step B).
Pre-existing on feature/v2.0, NOT introduced by this task:
- check-doxygen.sh fails with 7 warnings (4 @security + start()
refs + stale arguments_accumulator refs) in
create_webserver.hpp / hook_context.hpp / webserver_websocket.hpp.
None of those files are in this diff; tracked separately for a
future cleanup.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds Sub-test D (per_resource_add_hook_first_call_cas_no_data_race) and
extends Sub-test A's driver_body switch from 6 ops to 8 ops to cover the
per-resource (middle-tier) hook bus that the existing test header was
claiming but not actually exercising.
Sub-test D structurally proves the contended-null window in
http_resource::ensure_table() (src/http_resource.cpp:93-110) is entered:
64 iterations x 8 racing threads, each iteration synchronised on a
std::latch so workers enter add_hook concurrently against a freshly-null
hook_table_ slot. The HTTPSERVER_COMPILATION-gated hook_table_raw_()
accessor witnesses pre-race null (none installed) -> post-race non-null
(exactly one CAS-arbitrated table installed). A per-iteration
"entered-after-latch" counter pins the structural CAS-contention proof
(>=2 workers visible inside the contended-null window). Total CAS races
per run: 512 (shape-bounded; ~5ms warm, ~25ms TSan).
Sub-test A's driver_body now also fires:
- op 6: fresh stack-local cas_witness_resource per call -> contended-null
branch of ensure_table() driven from inside an MHD handler thread while
route_table_mutex_ is held shared by dispatch (the documented 3-tier
lock order in a single moment).
- op 7: shared cas_witness_resource captured by reference -> load-acquire
short-circuit branch ("if (existing) return existing;") complementing
op 6's contended-null branch.
Two new counters (cas_resource_hook_ok, cas_existing_hook_ok) are pinned
by LT_CHECK_GT alongside the existing TASK-032/052 counters.
The Sub-test A header comment is corrected to stop overclaiming
per-resource coverage; an explicit Sub-test D header describes the new
coverage. grep -n "resource hook_table" returns three comment lines
accurately describing the lock-order claim (AC 3).
src/http_resource.cpp is bit-identical (AC 4). No library rebuild
required (HTTPSERVER_COMPILATION already in test/Makefile.am AM_CPPFLAGS).
No instrumentation macro added; the witness is purely test-side.
Manual TSan validation on macOS (Apple clang) confirmed zero data-race
warnings; the CI Linux libstdc++ TSan lane re-runs this binary via the
existing build-type: tsan matrix entry in .github/workflows/verify-build.yml
without workflow changes.
Local: HTTPSERVER_STRESS_SECONDS=2 make check -j1 -> 98/98 PASS.
Sub-test D INFO line: iters=64 threads_per_iter=8
total_post_race_nonnull=64 contended_window_iters=64.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ct and mark Done - Add CONTRACT comment to hook_table_raw_() in http_resource.hpp making the synchronisation requirement explicit: the accessor is a non-atomic .get() on the shared_ptr and is safe only when called after a happens-before edge over any concurrent ensure_table() writer (e.g. after joining all threads). Addresses security-reviewer-iter1-1. - Add companion comment at the Sub-test D call site (line 993) explaining why the post-join read is safe. Addresses security-reviewer-iter1-1. - Create TASK-094.md in worktree and mark Status: Done with all six action item checkboxes checked. Addresses housekeeper-iter1-1 and housekeeper-iter1-2. - Add TASK-094 row (Done) to _index.md and bump Last updated to 2026-06-07. Addresses housekeeper-iter1-3. - Add cross-reference note to TASK-070.md under the TSan acceptance criterion and in the Dependencies section. Addresses housekeeper-iter1-4. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… CAS race Co-Authored-By: Claude Opus 4.7 <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>
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