Skip to content

Commit 8dfb558

Browse files
etrclaude
andcommitted
TASK-056 validation iter1: address reviewer findings
Housekeeper: - Mark all five TASK-056 action items complete ([x]) in v2-deferred-backlog-plan.md - Add 'Status: Done' to TASK-056 section (consistent with TASK-054/055 pattern) - Correct routing.md -> route-table.md filename in action item text test-quality-reviewer (major): - threadsafety_stress: add zero-floor guard (baseline = max(warmup_median, 1000ns)) so the latency gate never becomes 'p99 < 0' on sub-microsecond hosts - routing_regression_test: remove internal tier/is_prefix field assertions from all six collision tests; replace with observable-behaviour checks (does the surviving route resolve expected paths, does the failed registration leave no prefix terminus) performance-reviewer (major): - radix_tree: change insert/remove/has_terminus_at and tokenize() from std::string_view to const std::string& so callers holding a const std::string& avoid the extra copy through the string_view conversion when calling tokenize_url - webserver_dispatch: fix misleading comment on the cache-miss copy path; clarify that a full copy (not move) is required because result is returned by value and moving captured_params out would silently empty the return value Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 50525b4 commit 8dfb558

5 files changed

Lines changed: 67 additions & 30 deletions

File tree

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

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -222,20 +222,20 @@ TASK-027 cleanup:
222222
collide on the cache key.
223223

224224
**Action Items:**
225-
- [ ] Replace `std::unordered_map` with `std::map` (or a small
225+
- [x] Replace `std::unordered_map` with `std::map` (or a small
226226
flat_map) in the radix node child container. Benchmark the impact:
227227
acceptable if cache-miss path stays under 5 µs on the worst-case
228228
realistic tree depth.
229-
- [ ] Add the `is_prefix_match` guard at the call site of
229+
- [x] Add the `is_prefix_match` guard at the call site of
230230
`upsert_v2_param_route` so prefix-vs-exact terminus collisions are
231231
detected at registration time, not at lookup time.
232-
- [ ] Add a regression test
232+
- [x] Add a regression test
233233
`routing_regression_test.cpp::register_exact_after_prefix_does_not_collide`
234234
pinning the registration-time error.
235-
- [ ] Add a stress-test variant in `threadsafety_stress.cpp` that
235+
- [x] Add a stress-test variant in `threadsafety_stress.cpp` that
236236
hammers the registration path with adversarial path segments to
237237
confirm the new container is DoS-resistant.
238-
- [ ] Update `specs/architecture/04-components/routing.md` with the
238+
- [x] Update `specs/architecture/04-components/route-table.md` with the
239239
new container choice and rationale.
240240

241241
**Dependencies:**
@@ -253,6 +253,8 @@ TASK-027 cleanup:
253253
**Related Findings:** task-027 #14, task-027 #18
254254
**Related Decisions:** §4.5 routing
255255

256+
**Status:** Done
257+
256258
---
257259

258260
## TASK-057 — Redact credentials in `http_request::operator<<`

src/detail/webserver_dispatch.cpp

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -203,10 +203,17 @@ webserver_impl::lookup_v2(http_method method, const std::string& path) {
203203
}
204204
} // table_lock released.
205205

206-
// Step 3: install into cache (cache mutex only). Copy result.entry
207-
// and result.captured_params into the cache_value — the caller
208-
// consumes `result` after this returns, so a move-out would leave
209-
// the caller reading a moved-from variant / empty captures vector.
206+
// Step 3: install into cache (cache mutex only). Full copy of
207+
// result.entry and result.captured_params into cache_value v, then
208+
// return result by value (NRVO).
209+
//
210+
// Why copy and not move:
211+
// - result is returned by value on the next line; moving any field
212+
// out of it here would return a partially moved-from struct to
213+
// the caller, silently losing captured_params.
214+
// - result.entry carries a shared_ptr<http_resource> variant; the
215+
// copy atomically bumps the refcount once (unavoidable regardless
216+
// of move-or-copy for the cache slot).
210217
// (The same defensive copy lands in TASK-053; if TASK-053 merges
211218
// first this hunk becomes a benign duplicate.)
212219
if (result.found) {

src/httpserver/detail/radix_tree.hpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ class radix_tree {
103103
// (webserver_impl) is responsible for keeping the is_prefix argument
104104
// consistent with route_entry::is_prefix, which is the §4.7 source
105105
// of truth. Replaces an existing terminus of the same kind.
106-
void insert(std::string_view path, T entry, bool is_prefix = false) {
106+
void insert(const std::string& path, T entry, bool is_prefix = false) {
107107
radix_node<T>* node = root_.get();
108108
const auto segments = tokenize(path);
109109
for (const std::string& seg : segments) {
@@ -225,7 +225,7 @@ class radix_tree {
225225
// inserting a NEW exact terminus at /admin we need to refuse if a
226226
// prefix terminus is already registered at /admin (and vice versa)
227227
// — silent shadowing would corrupt the (method, path) cache key.
228-
bool has_terminus_at(std::string_view path, bool is_prefix) const {
228+
bool has_terminus_at(const std::string& path, bool is_prefix) const {
229229
const radix_node<T>* node = root_.get();
230230
const auto segments = tokenize(path);
231231
for (const std::string& seg : segments) {
@@ -249,7 +249,7 @@ class radix_tree {
249249
// NOTE: unlike find(), where descent uses the concrete request-path
250250
// segment value (e.g. "42"), remove() receives the registered pattern
251251
// (e.g. "{id}") and matches wildcard nodes by the placeholder shape.
252-
bool remove(std::string_view path, bool is_prefix) {
252+
bool remove(const std::string& path, bool is_prefix) {
253253
radix_node<T>* node = root_.get();
254254
const auto segments = tokenize(path);
255255
for (const std::string& seg : segments) {
@@ -285,10 +285,10 @@ class radix_tree {
285285
}
286286

287287
private:
288-
static std::vector<std::string> tokenize(std::string_view path) {
289-
// tokenize_url takes a std::string by value via string_split; copy
290-
// the view's contents to call it.
291-
return ::httpserver::http::http_utils::tokenize_url(std::string{path});
288+
static std::vector<std::string> tokenize(const std::string& path) {
289+
// tokenize_url takes std::string by value; pass the already-owned
290+
// string to avoid an extra copy when callers hold a const string&.
291+
return ::httpserver::http::http_utils::tokenize_url(path);
292292
}
293293

294294
static bool is_wildcard_segment(const std::string& seg) noexcept {

test/integ/threadsafety_stress.cpp

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -755,7 +755,16 @@ LT_BEGIN_AUTO_TEST(threadsafety_stress_suite,
755755
// Gate: p99 must not exceed 10× the warmup median. With std::map
756756
// (O(log n) per probe), the ratio in practice is < 3×; the 10×
757757
// gate gives the test margin under CI noise.
758-
LT_CHECK_LT(p99, warmup_median * 10);
758+
//
759+
// Zero-floor guard: on sub-microsecond hosts or with a coarse
760+
// steady_clock quantum the OS may quantise all warmup samples to 0 ns,
761+
// making warmup_median == 0 and therefore warmup_median * 10 == 0,
762+
// which turns the gate into `p99 < 0` — an impossible assertion that
763+
// always fails. Clamp the baseline to at least 1 µs so the gate
764+
// remains a meaningful "no spike" check even on very fast hardware.
765+
const int64_t baseline = std::max(warmup_median,
766+
static_cast<int64_t>(1000));
767+
LT_CHECK_LT(p99, baseline * 10);
759768

760769
ws.stop();
761770
LT_END_AUTO_TEST(adversarial_segments_registration_no_latency_spike)

test/unit/routing_regression_test.cpp

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -369,11 +369,15 @@ LT_BEGIN_AUTO_TEST(routing_regression_suite,
369369
}
370370
LT_CHECK(threw);
371371
// Original prefix entry must remain intact (atomicity contract).
372+
// Verify via observable behaviour: /admin/anything resolves (prefix
373+
// semantics) and /admin also resolves (the prefix terminates here).
372374
auto r = impl_of(ws).lookup_v2(
373375
ht::http_method::get, std::string("/admin/anything"));
374376
LT_CHECK(r.found);
375-
LT_CHECK(r.tier == ht::detail::webserver_impl::tier_hit::radix);
376-
LT_CHECK(r.entry.is_prefix);
377+
// Bare path also resolves via prefix.
378+
auto r2 = impl_of(ws).lookup_v2(
379+
ht::http_method::get, std::string("/admin"));
380+
LT_CHECK(r2.found);
377381
LT_END_AUTO_TEST(register_exact_after_prefix_does_not_collide)
378382

379383
LT_BEGIN_AUTO_TEST(routing_regression_suite,
@@ -391,14 +395,12 @@ LT_BEGIN_AUTO_TEST(routing_regression_suite,
391395
}
392396
LT_CHECK(threw);
393397
// Original exact entry must remain intact: the bare path still
394-
// resolves through the exact tier.
398+
// resolves (observable behaviour — exact match exists).
395399
auto r = impl_of(ws).lookup_v2(
396400
ht::http_method::get, std::string("/admin"));
397401
LT_CHECK(r.found);
398-
LT_CHECK(r.tier == ht::detail::webserver_impl::tier_hit::exact);
399-
LT_CHECK(!r.entry.is_prefix);
400402
// And the failed prefix registration must NOT have planted a
401-
// prefix terminus — /admin/sub must miss.
403+
// prefix terminus — /admin/sub must miss (exact semantics preserved).
402404
auto sub = impl_of(ws).lookup_v2(
403405
ht::http_method::get, std::string("/admin/sub"));
404406
LT_CHECK(!sub.found);
@@ -417,10 +419,15 @@ LT_BEGIN_AUTO_TEST(routing_regression_suite,
417419
threw = true;
418420
}
419421
LT_CHECK(threw);
422+
// Original prefix entry must remain intact: /static/x resolves
423+
// (prefix semantics — deeper path matches), and /static itself also
424+
// resolves (the prefix terminus is anchored here).
420425
auto r = impl_of(ws).lookup_v2(
421426
ht::http_method::get, std::string("/static/x"));
422427
LT_CHECK(r.found);
423-
LT_CHECK(r.entry.is_prefix);
428+
auto r2 = impl_of(ws).lookup_v2(
429+
ht::http_method::get, std::string("/static"));
430+
LT_CHECK(r2.found);
424431
LT_END_AUTO_TEST(register_path_after_prefix_does_not_collide)
425432

426433
LT_BEGIN_AUTO_TEST(routing_regression_suite,
@@ -436,10 +443,14 @@ LT_BEGIN_AUTO_TEST(routing_regression_suite,
436443
threw = true;
437444
}
438445
LT_CHECK(threw);
446+
// Original exact entry must remain intact: /static resolves, but
447+
// /static/sub does NOT (exact semantics — no deeper path matches).
439448
auto r = impl_of(ws).lookup_v2(
440449
ht::http_method::get, std::string("/static"));
441450
LT_CHECK(r.found);
442-
LT_CHECK(!r.entry.is_prefix);
451+
auto sub = impl_of(ws).lookup_v2(
452+
ht::http_method::get, std::string("/static/sub"));
453+
LT_CHECK(!sub.found);
443454
LT_END_AUTO_TEST(register_prefix_after_path_does_not_collide)
444455

445456
LT_BEGIN_AUTO_TEST(routing_regression_suite,
@@ -458,12 +469,16 @@ LT_BEGIN_AUTO_TEST(routing_regression_suite,
458469
threw = true;
459470
}
460471
LT_CHECK(threw);
461-
// The original prefix entry must still match a deeper path.
472+
// The original prefix entry must still match a deeper path
473+
// (observable behaviour: /users/42/profile resolves via prefix).
462474
auto r = impl_of(ws).lookup_v2(
463475
ht::http_method::get, std::string("/users/42/profile"));
464476
LT_CHECK(r.found);
465-
LT_CHECK(r.tier == ht::detail::webserver_impl::tier_hit::radix);
466-
LT_CHECK(r.entry.is_prefix);
477+
// The exact node itself also resolves (prefix captures /users/42 and
478+
// any extension).
479+
auto r2 = impl_of(ws).lookup_v2(
480+
ht::http_method::get, std::string("/users/42"));
481+
LT_CHECK(r2.found);
467482
LT_END_AUTO_TEST(parameterized_exact_after_parameterized_prefix_does_not_collide)
468483

469484
LT_BEGIN_AUTO_TEST(routing_regression_suite,
@@ -480,11 +495,15 @@ LT_BEGIN_AUTO_TEST(routing_regression_suite,
480495
threw = true;
481496
}
482497
LT_CHECK(threw);
498+
// Original exact entry must remain intact: /users/42 resolves.
483499
auto r = impl_of(ws).lookup_v2(
484500
ht::http_method::get, std::string("/users/42"));
485501
LT_CHECK(r.found);
486-
LT_CHECK(r.tier == ht::detail::webserver_impl::tier_hit::radix);
487-
LT_CHECK(!r.entry.is_prefix);
502+
// The failed prefix registration must NOT have planted a prefix
503+
// terminus — /users/42/profile must miss (exact semantics).
504+
auto sub = impl_of(ws).lookup_v2(
505+
ht::http_method::get, std::string("/users/42/profile"));
506+
LT_CHECK(!sub.found);
488507
LT_END_AUTO_TEST(parameterized_prefix_after_parameterized_exact_does_not_collide)
489508

490509
// ---------------------------------------------------------------------

0 commit comments

Comments
 (0)