Skip to content

Commit 9789710

Browse files
etrclaude
andcommitted
TASK-094: address review findings — document hook_table_raw_() contract 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>
1 parent ed85a1c commit 9789710

5 files changed

Lines changed: 60 additions & 2 deletions

File tree

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,15 @@
1515
- [ ] Remove the TODO comment.
1616

1717
**Dependencies:**
18-
- Blocked by: TASK-001 (C++20 floor, Done), TASK-051 (per-route hooks, Done)
18+
- Blocked by: TASK-001 (C++20 floor, Done), TASK-051 (per-route hooks, Done), libc++ P0718R2 (Apple Clang/libc++ still lacks `std::atomic<std::shared_ptr<T>>` — recheck each LLVM release; see project memory)
1919
- Blocks: None
20+
- Spun off: TASK-094 (Done) — delivers the TSan stress harness for the per-resource CAS path ahead of the migration
2021

2122
**Acceptance Criteria:**
2223
- `grep -nE 'atomic_load|atomic_store' src/http_resource.cpp` returns no matches.
2324
- `grep -nE 'Wdeprecated-declarations' src/http_resource.cpp` returns no matches.
2425
- Stress test `threadsafety_stress` extended with hook table swap remains TSan-clean.
26+
*(Satisfied out-of-band by TASK-094 — Done. TASK-094 added Sub-test D that stress-tests the legacy CAS path under TSan. When TASK-070 eventually lands, Sub-test D is the required regression net for the migrated atomic path.)*
2527
- Typecheck passes.
2628
- Tests pass.
2729

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
### TASK-094: Extend `threadsafety_stress` with per-resource `add_hook` CAS race
2+
3+
**Milestone:** M7 - v2 Cleanup
4+
**Component:** `test/integ/threadsafety_stress.cpp`, `src/http_resource.cpp`
5+
**Estimate:** S
6+
7+
**Goal:**
8+
Spun off from TASK-070, which is blocked on libc++ implementing P0718R2 (see TASK-070 "Blocker" section). One acceptance-criterion piece of TASK-070 — *"Stress test `threadsafety_stress` extended with hook table swap remains TSan-clean"* — does **not** depend on the migration and can land today. The existing stress test (`test/integ/threadsafety_stress.cpp`, comment at lines 25–30) claims to cover the `route_table_mutex_ → resource hook_table → server-wide hook_table` lock order under TSan, but in practice it only hammers `webserver::add_hook` + `hook_handle::remove` (the M5/TASK-052 server-side surface). The per-resource lazy CAS path in `src/http_resource.cpp:93-110` (`ensure_table()` — `std::atomic_load_explicit` + `std::atomic_compare_exchange_strong_explicit` on a `std::shared_ptr<detail::resource_hook_table>` field) is never driven by the test. This task closes that gap so we (a) gain regression coverage on the current legacy implementation **today** and (b) seed a ready-made stress harness for the day TASK-070 unblocks.
9+
10+
**Action Items:**
11+
- [x] Add a new stress phase to `test/integ/threadsafety_stress.cpp` that targets the lazy CAS path in `http_resource::ensure_table()` (`src/http_resource.cpp:93-110`). The phase must repeatedly: (a) construct a fresh `http_resource` subclass whose `hook_table_` is null, (b) launch N concurrent threads (≥8) that each call `http_resource::add_hook` so they race the `atomic_compare_exchange_strong_explicit` on a null slot, (c) follow with mixed add/remove load against the now-installed table to exercise concurrent registration + dispatch.
12+
- [x] Sequence the new phase alongside the existing webserver-side hook ops so a single run exercises both lock-order tiers (`webserver::add_hook``http_resource::add_hook`).
13+
- [x] Verify the phase actually drives `ensure_table()` (e.g., assert that at least one CAS loser occurs across the run — via either a debug counter behind `HTTPSERVER_TEST_INSTRUMENTATION` or by inspecting `hook_table_raw_()` from a witness thread). Acceptable if the assertion only runs in TSan/instrumented lanes.
14+
- [x] Run the extended `threadsafety_stress` under the existing TSan lane and confirm zero warnings.
15+
- [x] Update the test header comment (lines 23–34) to accurately describe the resource-side coverage that was previously claimed but not delivered.
16+
- [x] Do **not** modify `src/http_resource.cpp` semantics. The migration stays parked in TASK-070; this task is test-only (plus the optional test-only instrumentation counter, if used).
17+
18+
**Dependencies:**
19+
- Blocked by: None (the legacy CAS path is what this task stresses; it exists today).
20+
- Blocks: None. Unblocks one acceptance criterion of TASK-070 ahead of time — when TASK-070 eventually lands, this stress phase is the regression net it requires.
21+
22+
**Acceptance Criteria:**
23+
- Extended `threadsafety_stress` runs TSan-clean under the existing CI matrix (Linux libstdc++ TSan lane is authoritative; macOS lane informational).
24+
- New phase observably drives the CAS in `ensure_table()`: either an instrumented counter shows ≥1 CAS-loser across a representative run, or a documented design rationale explains why the schedule provably hits the contended-null window.
25+
- `grep -n "resource hook_table" test/integ/threadsafety_stress.cpp` returns at least one comment line that accurately describes the new coverage (replacing the previous overclaim).
26+
- `src/http_resource.cpp` is unchanged except for at most one optional `#ifdef HTTPSERVER_TEST_INSTRUMENTATION` counter increment in `ensure_table()` if the visibility approach chosen needs it.
27+
- Typecheck passes.
28+
- All tests pass.
29+
30+
**Related Requirements:** PRD §2 modern C++ NFR; PRD §5 concurrency NFR (lock-order documentation).
31+
**Related Decisions:** DR-001 (C++20); the `route_table_mutex_ → resource hook_table → server-wide hook_table` lock order documented in `specs/architecture/05-cross-cutting.md` §5.6 (recorded by TASK-051).
32+
33+
**Status:** Done

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
# M7 — v2.0 Branch Cleanup
22

33
**Status:** Draft 1
4-
**Last updated:** 2026-06-04
4+
**Last updated:** 2026-06-07
55
**Owner:** Sebastiano Merlino
66
**Inputs:** [../v2-branch-gap-audit.md](../v2-branch-gap-audit.md)
77

@@ -58,6 +58,7 @@ TASK-093).
5858
| TASK-091 | Tighten CI script soft-degradations | MED | M | Backlog |
5959
| TASK-092 | Wire `route_table_concurrency` TSan + `stop()`-from-handler into per-PR CI | MED | S | Backlog |
6060
| TASK-093 | Tighten example caveats (auth, pipe, access-log) | LOW | S | Backlog |
61+
| TASK-094 | Extend `threadsafety_stress` with per-resource `add_hook` CAS race | MED | S | Done |
6162

6263
## Dependency notes
6364

src/httpserver/http_resource.hpp

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,19 @@ class http_resource {
394394
// hooks for any phase"). Public-but-HTTPSERVER_COMPILATION-gated
395395
// for the same reason webserver::make_hook_handle_ is: the symbol
396396
// is reachable only from within the library translation units.
397+
//
398+
// CONTRACT — synchronisation requirement:
399+
// This is a **non-atomic** read of the shared_ptr's stored pointer
400+
// via std::shared_ptr::get(). It is safe only when the caller
401+
// holds a happens-before edge over any concurrent writer of
402+
// hook_table_: e.g., after all mutating threads have been joined
403+
// or after an acquire fence/lock that sequenced-after the last
404+
// atomic_store in ensure_table(). Do NOT call this from a thread
405+
// that races with ensure_table() without external synchronisation.
406+
// (ensure_table() itself uses the deprecated free-function
407+
// std::atomic_*_explicit overloads; once TASK-070 migrates the
408+
// field to std::atomic<std::shared_ptr<T>>, an acquire load should
409+
// replace the .get() call here too.)
397410
detail::resource_hook_table* hook_table_raw_() const noexcept {
398411
return hook_table_.get();
399412
}

test/integ/threadsafety_stress.cpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -990,6 +990,15 @@ LT_BEGIN_AUTO_TEST(threadsafety_stress_suite,
990990
contended_window_iters.fetch_add(
991991
1, std::memory_order_relaxed);
992992
}
993+
// Post-race witness: safe non-atomic .get() read because all
994+
// workers and burst threads have been joined above (both
995+
// workers.join() and burst.join() loops complete, and
996+
// handles.clear() has drained the handle bag). The join()
997+
// calls establish a happens-before edge over every atomic store
998+
// in ensure_table(), making the non-atomic hook_table_.get()
999+
// inside hook_table_raw_() observationally correct here.
1000+
// (See the CONTRACT comment on hook_table_raw_() in
1001+
// src/httpserver/http_resource.hpp for the full rationale.)
9931002
if (r.hook_table_raw_() != nullptr) {
9941003
total_post_race_nonnull.fetch_add(
9951004
1, std::memory_order_relaxed);

0 commit comments

Comments
 (0)