Skip to content

Commit a0475f4

Browse files
etrclaude
andcommitted
TASK-078: resolve commented-out CONNECT bodies and dead validator integ test
Two HIGH audit findings in test/integ/basic.cpp: (a) Two `/* ... */`-commented CONNECT-method test bodies in basic_suite::complete and basic_suite::only_render, dating to commit bd39cb4 (Nov 2017, "Avoid tests getting stack on CONNECT requests"). The hang was a libcurl client-side artifact - curl_easy_perform with CURLOPT_CUSTOMREQUEST "CONNECT" waits for a tunnel that never comes from a plain HTTP server. Server-side CONNECT dispatch IS exercised: webserver_request.cpp's methods[] table maps the CONNECT wire token to render_connect, and the render_connect signature is pinned by test/unit/http_resource_test.cpp. Delete the dead bodies and record the rationale as REGRESSION.md divergence #6. (b) basic_suite::validator_builder only asserted "server boots with a validator callback set" - the validator hook has been unwired from the dispatch path since commit 9163a4f (Jan 2013, "Eliminated unescaper and validator delegates"). The equivalent compile-time pin already lives in test/unit/create_webserver_test.cpp::builder_validator (LT_CHECK_NOTHROW on the builder call). Delete the integ test and note in RELEASE_NOTES.md that v2's request_received / accept_decision hooks are the documented replacement for the URL-veto use case the original validator was intended for. No public surface change. The validator() builder method, validator_ptr typedef, and get_request_validator() accessor remain as inert v1 surface (separate task if anyone wants to actually remove them). The basic test count drops by 1 (validator_builder removed, 97 -> 96); the complete and only_render tests pass unchanged. Full make check: 103/103 PASS. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 4a9a2b5 commit a0475f4

5 files changed

Lines changed: 43 additions & 61 deletions

File tree

RELEASE_NOTES.md

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,22 @@ v1.x is end-of-life on the day v2.0 ships.
8080
the legacy callable is no longer implicitly convertible to
8181
`auth_handler_ptr`. There is no runtime ABI surface to break — the
8282
overload was a header-only inline forwarder.
83+
- **`create_webserver::validator(...)` dispatch semantics — confirmed
84+
never wired (TASK-078).** The `validator` builder method and the
85+
`validator_ptr` callback typedef on `create_webserver` are retained
86+
as inert v1 surface — the callback has not been invoked from the
87+
dispatch path since commit `9163a4f` (Jan 2013, "Eliminated unescaper
88+
and validator delegates"). v2 ships the surface unchanged for source
89+
compatibility, but a configured validator callback is dead code. The
90+
v2 replacement is `webserver::add_hook(hook_phase::request_received,
91+
...)` returning `hook_action::respond_with(http_response)` to reject
92+
a request, or `hook_phase::accept_decision` for a connection-scoped
93+
veto (see `specs/architecture/04-components/hooks.md`). The legacy
94+
`validator_builder` integ test in `test/integ/basic.cpp` exercised
95+
no validation behaviour (it only asserted the server booted with a
96+
validator set) and has been removed; the equivalent compile-time
97+
builder pin survives at
98+
`test/unit/create_webserver_test.cpp::builder_validator`.
8399
- **v1 `registered_resources*` registration maps (internal).** No
84100
public-API change. Dispatch already routed through the v2 3-tier
85101
route table (`lookup_v2()`) after TASK-053; the residual

specs/tasks/M7-v2-cleanup/TASK-078.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@
88
`test/integ/basic.cpp:821-830, 885-896` carry `/* … */`-commented-out CONNECT-method test bodies with no tracking issue. `basic.cpp:2895` (`validator_builder` test) only asserts the server boots; the validator hook is "stored but not currently called in webserver". Either restore the bodies or remove them with a tracking link.
99

1010
**Action Items:**
11-
- [ ] Investigate why the CONNECT-method blocks at `basic.cpp:821-830` and `885-896` were commented out (git log of the lines). If the commented behaviour is no longer achievable under v2 dispatch, delete the blocks and add a one-line note to `test/REGRESSION.md` recording the v1-only feature.
12-
- [ ] If the blocks should still pass under v2, uncomment them and fix any breakage.
13-
- [ ] For the `validator_builder` test at `basic.cpp:2895`: either wire the validator hook into the dispatch path so the test exercises real behaviour, or delete the test and note in `RELEASE_NOTES.md` that `validator_builder` is a v1 surface with no v2 semantics.
11+
- [x] Investigate why the CONNECT-method blocks at `basic.cpp:821-830` and `885-896` were commented out (git log of the lines). If the commented behaviour is no longer achievable under v2 dispatch, delete the blocks and add a one-line note to `test/REGRESSION.md` recording the v1-only feature.
12+
- [x] If the blocks should still pass under v2, uncomment them and fix any breakage.
13+
- [x] For the `validator_builder` test at `basic.cpp:2895`: either wire the validator hook into the dispatch path so the test exercises real behaviour, or delete the test and note in `RELEASE_NOTES.md` that `validator_builder` is a v1 surface with no v2 semantics.
1414

1515
**Dependencies:**
1616
- Blocked by: None
@@ -25,4 +25,4 @@
2525
**Related Requirements:** PRD §2 test reliability NFR
2626
**Related Decisions:** None new
2727

28-
**Status:** Backlog
28+
**Status:** Done

specs/tasks/M7-v2-cleanup/_index.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ TASK-093).
4242
| TASK-075 | Propagate `wait_for_server_ready` to sibling hooks integ tests | HIGH | M | Done |
4343
| TASK-076 | Replace tautological-pass blocks in TLS test lanes | HIGH | M | Done |
4444
| TASK-077 | Restore Windows / Darwin coverage in skipped test suites | HIGH | L | Done |
45-
| TASK-078 | Resolve commented-out CONNECT-method test bodies | HIGH | S | Backlog |
45+
| TASK-078 | Resolve commented-out CONNECT-method test bodies | HIGH | S | Done |
4646
| TASK-079 | Drive nonce/opaque state machine in v2 digest-auth integ tests | MED | M | Backlog |
4747
| TASK-080 | Tighten threadsafety_stress latency gate back from 100× to 10× | MED | M | Backlog |
4848
| TASK-081 | Fill empty-on-correct-build unit suites and re-enable pthread leak detector | MED | M | Backlog |

test/REGRESSION.md

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,28 @@ it at the head of `lookup_v2`, including cache keying. This brings v2
164164
in line with v1 semantics; the test
165165
`exact_path_normalization_aliases` pins it.
166166

167+
### 6. CONNECT-method client-roundtrip integ test (TASK-078)
168+
169+
`test/integ/basic.cpp` previously contained two `/* ... */`-commented
170+
CONNECT bodies inside `basic_suite::complete` and `basic_suite::only_render`
171+
that issued `curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "CONNECT")`
172+
through the high-level libcurl easy API. They were commented out in
173+
2017 (commit bd39cb4) because libcurl treats CONNECT as a tunnel-establishing
174+
verb and waits for the socket to be upgraded; `curl_easy_perform` then
175+
hangs against a plain HTTP server. The bodies have been deleted; there is
176+
no way to drive a CONNECT round-trip through `curl_easy_perform` and the
177+
high-level easy API has no other shape that produces a useful integ
178+
assertion against an HTTP server that is not also a proxy.
179+
180+
Server-side CONNECT dispatch IS exercised: the method-to-render-fn table
181+
in `src/detail/webserver_request.cpp` (the `methods[]` array around line
182+
608) maps the `CONNECT` wire token to `http_resource::render_connect`,
183+
and `test/unit/http_resource_test.cpp::render_connect_returns_by_value`
184+
pins the public signature. The deleted integ blocks added no coverage
185+
beyond that.
186+
187+
No port required; not a v1-only feature, but a v1-era libcurl misuse.
188+
167189
## How to extend
168190

169191
When adding a new routing pattern to the public API:

test/integ/basic.cpp

Lines changed: 0 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -833,16 +833,6 @@ LT_BEGIN_AUTO_TEST(basic_suite, complete)
833833
LT_ASSERT_EQ(res, 0);
834834
curl_easy_cleanup(curl);
835835
}
836-
/*
837-
{
838-
CURL* curl = curl_easy_init();
839-
curl_easy_setopt(curl, CURLOPT_URL, "localhost:" PORT_STRING "/base");
840-
curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "CONNECT");
841-
CURLcode res = curl_easy_perform(curl);
842-
LT_ASSERT_EQ(res, 0);
843-
curl_easy_cleanup(curl);
844-
}
845-
*/
846836

847837
{
848838
CURL* curl = curl_easy_init();
@@ -897,19 +887,6 @@ LT_BEGIN_AUTO_TEST(basic_suite, only_render)
897887
LT_CHECK_EQ(s, "OK");
898888
curl_easy_cleanup(curl);
899889

900-
/*
901-
s = "";
902-
curl = curl_easy_init();
903-
curl_easy_setopt(curl, CURLOPT_URL, "localhost:" PORT_STRING "/base");
904-
curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "CONNECT");
905-
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, writefunc);
906-
curl_easy_setopt(curl, CURLOPT_WRITEDATA, &s);
907-
res = curl_easy_perform(curl);
908-
LT_ASSERT_EQ(res, 0);
909-
LT_CHECK_EQ(s, "OK");
910-
curl_easy_cleanup(curl);
911-
*/
912-
913890
s = "";
914891
curl = curl_easy_init();
915892
curl_easy_setopt(curl, CURLOPT_URL, "localhost:" PORT_STRING "/base");
@@ -2907,39 +2884,6 @@ LT_BEGIN_AUTO_TEST(basic_suite, single_resource_mode)
29072884
ws2.stop();
29082885
LT_END_AUTO_TEST(single_resource_mode)
29092886

2910-
// Test validator builder method (validator is stored but not currently called in webserver)
2911-
bool test_validator_func(const std::string& url) {
2912-
return url.find("valid") != std::string::npos;
2913-
}
2914-
2915-
LT_BEGIN_AUTO_TEST(basic_suite, validator_builder)
2916-
// Test that the validator builder method works (for coverage of create_webserver.hpp)
2917-
webserver ws2{create_webserver(PORT + 72)
2918-
.validator(test_validator_func)};
2919-
auto resource = std::make_shared<ok_resource>();
2920-
ws2.register_path("test", resource);
2921-
ws2.start(false);
2922-
2923-
// Just verify the server works with a validator set
2924-
{
2925-
curl_global_init(CURL_GLOBAL_ALL);
2926-
string s;
2927-
CURL *curl = curl_easy_init();
2928-
CURLcode res;
2929-
std::string url = "http://localhost:" + std::to_string(PORT + 72) + "/test";
2930-
curl_easy_setopt(curl, CURLOPT_URL, url.c_str());
2931-
curl_easy_setopt(curl, CURLOPT_HTTPGET, 1L);
2932-
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, writefunc);
2933-
curl_easy_setopt(curl, CURLOPT_WRITEDATA, &s);
2934-
res = curl_easy_perform(curl);
2935-
LT_ASSERT_EQ(res, 0);
2936-
LT_CHECK_EQ(s, "OK");
2937-
curl_easy_cleanup(curl);
2938-
}
2939-
2940-
ws2.stop();
2941-
LT_END_AUTO_TEST(validator_builder)
2942-
29432887
// Test resource with no render methods overridden (exercises empty_render path)
29442888
// Note: empty_render returns string_response with code -1, which triggers internal error
29452889
class empty_render_resource : public http_resource {

0 commit comments

Comments
 (0)